[Regression/Refactor] Restore Ajax link type "flag message" functionality

I am posting an early early draft

I am just showing the basic structure/outline that can be seen in "proposed solution" of the issue summary.

What remains is to correct the bugs/flaws in the twig template / javascript interaction.

1) I've copied over the html element from flag.tpl.php into flag.html.twig
2) I've copied over the critical class array definitions from flag.module template_preprocess_flag() into flag.html.twig
3) I've converted flag,js drupal.settings to drupalSettings and fixed minor nits highlighted by passing the javascript through core's eslint rules.

PS I need to give much more care and attention to the handling of anonymous users.

more tomorrow night.

@rbayliss thank you for you patch, it seems a reasonable modification to the existing code.

When there are problems with the underlying architecture, I often find it takes a few iterations of the solution to draw focus on the real questions and that for me is the goodness in your patch.... but I think I want to change direction completely.

Let me highlight the code execution path for an ajax request

'Routing" -> ActionLinkController::[un])flag() -> ActionLinkController::generateResponse() -> ActionLinkTypeBase::getLink() -> ActionLinkTypeBase::buildLink() -> 'Theme Stuff"

Reviewing how the $ajax_message variable flows along this path ... I can see two classes of code smell.

1) There are lots of context handling if statements in the existing code which I want to refactor ( we are pre-alpha at the moment )

I think the context handling decisions are best made for free in the router and then the "routing function" drops control into one of four methods

a) handle flag ( normal )
b) handle unflag ( normal )
b) ) handle flag ( ajax )
c) handle unflag ( ajax )

Ahem well by 'normal' I mean prehistoric ( pre web2.0 ) style links

Fixing this should drop the cyclomatic complexity down a peg or two.

2) The second code smell hides a bug

ActionLinkController::generateResponse()

    if ($request->get(MainContentViewSubscriber::WRAPPER_FORMAT) == 'drupal_ajax') {

The canonical drupal8 way of handling this is through a drupal ajax command -- and then specifying a link in the theme to trigger the command.
If we stick to a well trodden path then we automatically manage the corner case of a user-agent that has javascript disabled. Something our existing code will be buggy in processing.

Anyway I just wanted to signal my intent early ... it may spark ideas in peoples minds ... patch is coming....

Dropped a t on the floor.

This is very much work in progress -- still things to debug

but the shape of the new architecture is in place.

Naming things is hard ... and I am about to make a radical change, please understand, if this makes anyone uneasy I will take the time to revert or rename based on suggestions!

We use 'Flag' in too many separate contexts

1) The module name
2) for templating action links see flag.html.twig
3) for the action carried by action_link 'flag' or 'unflag'

The naming confusion can be lessened by templating 'that thing' as an action_link

Please note this solution does not add a wrapper to the ajax link
and the message functionality is restored for normal links.

I have fixed a few gross misconceptions and screw ups.

AJAX links are currently rejected as - 406 Not Acceptable

more tomorrow.

The message flashing now works .. so I am triggering testbot.

Gravity has reclaimed my efforts to remove all context sensing if statements from the controller. CSRF token considerations scuppered my planning.

Here are the things I am thinking about for the next iteration:

1) The styling of the message flashes looks a little clunky ... suggestions as always are welcome.

2) The performance of the ajax_action_link response is a little sluggish... at little profiling seems in order.

3) From a security perspective I have added comments after I reviewed the server side. On the client side I am still considering the potential damage of taking input from the outside and inserting it unfiltered into the DOM. For TLS communications, at least, man in the middle style attacks are harder to pull off - but that is something D8 or the flag module does not require.

4) I am thinking of add a few extract tests to stop any regression of the action link 'description' and 'message flashing'.

test still fail ... I need to look into why the link pop out of the contextual link section.. hmm

I have moved common code back into ActionLinkController::generateAjaxResponse to get rid of a call to \Drupal::service('renderer')

The client side code to implement ActionLinkFlashCommand not longer leaves stale DOM element in after they have been faded out!

Lots if minor fixups. - I have been more explicit in the comments about when to generate 500-Internal Server Errors.

The code goes through user authentication ... uncachable database manipulations but this ajax back end operation for me takes approx. 500ms to function.. so still sluggish.

Sorry for not posting for a week...

I have fixed the test, and cutback my out of scope attempts to split up the flag twig template into ajax and normal variant.

I am unassigning myself

There are 3 remaining tasks, IMHO all of which need a bit of a discussion

1) Testing

The testing of ajax action links ( see LinkTypeAjacTest::doUseAjaxFlag ) has lots of perfectly good xpath checks.... which failed to notice that the message flasher ajax command was not even implemented! - so our testing needs augmentation Can I characterise what is going on in core with regard to mink and ajax testing as "it is at an early stage and rapidly evolving" in that light can others comment on what is appropriate here?

2) Security

My remaining security concerns can be applied to any \Drupal\Core\Ajax\ReplaceCommand - which makes that a core issue? Comments please.

3) Performance

It seems sluggish to me ... but I think we are doing nothing more that standard drupal ajax patterns. do we just right it off as yeah that is what you can expect for a D8 site?

Now the alpha has been released - I am triaging beta blockers.

It seems to me this should be near the top of the list, because 'ajax' links should be the most common link type found out in the wild.
an I want to make improvements with the highest impact first.

I am tagging in Javascript in an effort to bring javascript people to this issue.

postoning on #2461107: Flag links output as fields need to be in a block-level element

Does moving straight through "need work" to "postpone" mean I get £200 -- Sorry can you tell I played Monopoly over Christmas!

This is just a reroll.

I am un-postponing with the intent of resolving some blocker issues here.
by adding a wrapper and fixing some css.

#2461107: Flag links output as fields need to be in a block-level element
#2842845: throbber on AJAX links is incorrectly positioned

Sorry, don't have time to focus on this ... as always the weekend looks better

A little bit of issue juggling --- It is 6 of one and half a dozen of the other...

The wrapper is going in here #2842845: throbber on AJAX links is incorrectly positioned

This fix can come after.

Cool thanks to the overnight commit spree this issue is open again.

socketwench++

1) For normal flag action links, only normal css files are added to the page.

I have added a new css file.

( Without css/js aggregation) So when a AJAX flag action link is used two css files are added to the page.

one with normal css styling and one with AJAX specific styling.

Bug report: our existing css does not appear on the page! -- spawning a side issue.

2) Now other issues have added a div wrapper.... $.after becomes $.append

3) I ran out new js through eslint and picked out minor nits.

4) The message element is not displayed on the same line as the link through css styling.

5) resolved whitespace and EOF issues.

testbot was away with the fairies the patch passes.

Needs Test

A perfectly reasonable thing to say... on the face of things LinkTypeAJAXTest seems a good candidate.... I will try and get to this over the weekend.

While I think about it...

A trivial first step on the road to proper testing here is a unit test to ensure the new ajax command is constructed/rendered properly.

Regarding #35 that issue has been resolved. as the div wrapper was added.

Sorry for the stilted conversation, this had to wait until the weekend

@joachim

Can we compare test environments, I want to track down why we see differences.

I am testing from a basic site install of 8.3.x ( updated daily ) - so the theme is default bartik

When the link is clicked the wait icon appears followed by a small delay the message text appear inline with the text and then fades out.
with any janky shifting.

Sorry all, After going back to the drupal7 branch .. it is a understatement to say my approach to this issue was wrong headed.

I was just copying the behaviour from page two of

admin/structure/flags/add - where changes to the link type result in a flashing ajax method.

I should have looked at the javascript code in D7 much earlier and ported it over... my bad.

Regarding the port ... it looks at first glance like we can use the new AjaxCommand to simplify some of the jQuery selectors
and still accommodate future trigger points for the issue that comes behind this one and adds javascript events.
When porting we also need to look at debouncing and handling failed xhr requests as the d7 code already does.

I will have more time later in the week to work on this...

@socketwench your patch does more than fix my naming mistake. - it delete vital classes.
I have started with #36 and changed ActionLinkFlagCommand.php to ActionLinkFlagCommandTest.php

Just collecting my thoughts ... while I look for the next thing to do on this issue....might as well do it in public

On a D7 site I am just extracting the HTML associated with flag action links

Step to reproduce :
On Drupal 7
I enabled the flag module
Create three flags - linked to articles first flag second flag etc.
create a article - view article.

Here is what I see.

<div class="link-wrapper">
  <ul class="links inline">
    <li class="flag-third_flag first">
      <span>
        <span class="flag-wrapper flag-third-flag flag-third-flag-1">
          <a href="/flag/flag/third_flag/1?destination=node/1&amp;token=7M2UP5rPZiKG77kfRG0SHqJZu_GnR4AhURlPB9hWBfA" title="third link description" class="flag flag-action flag-link-toggle" rel="nofollow">Third Flag this item</a>
          <span class="flag-throbber">&nbsp;</span>
        </span>
      </span>
    </li>
    <li class="flag-second_flag">
      <span>
        <span class="flag-wrapper flag-second-flag flag-second-flag-1">
          <a href="/flag/flag/second_flag/1?destination=node/1&amp;token=7M2UP5rPZiKG77kfRG0SHqJZu_GnR4AhURlPB9hWBfA" title="second flag link description" class="flag flag-action flag-link-toggle" rel="nofollow">Second Flag this item</a>
          <span class="flag-throbber">&nbsp;</span>
        </span>
      </span>
    </li>
    <li class="flag-first_flag last">
      <span>
        <span class="flag-wrapper flag-first-flag flag-first-flag-1">
          <a href="/flag/flag/first_flag/1?destination=node/1&amp;token=7M2UP5rPZiKG77kfRG0SHqJZu_GnR4AhURlPB9hWBfA" title="first flag description" class="flag flag-action flag-link-toggle" rel="nofollow">First Flag this item</a>
          <span class="flag-throbber">&nbsp;</span>
        </span>
      </span>
    </li>
  </ul>
</div>

I am just going to give a running commentary, as always I am still just taking microsteps

Bear in mind that some of that code is part of the node links.

My PHP was good enough to see that was coming from a preprocess function, but having had nothing to do with D7 I am giggling at myself when I say "I did not know they were called node-links" so thanks that makes it much easier to google :)

I included all the HTML elements because it was interesting to note how D7's defaults differ from D8. While I am marking differences ... one thing this issue needs to restore is that AJAX link should be the default as they are in D7.

In #46 I talked about debouncing - that too is a stupid thing to say - since we a using 'ajax-links' to invoke a standard pattern there can be only one ajax request in flight at once -- so no need to copy over the use of the flag-waiting class.

Just for the record the debouncing, in effect, happens in Drupal.Ajax.prototype.eventResponse().

In #46 I also talked about restoring AJAX error handling ...
D7 removes a now defunct flag-waiting and pops an alert on the screen ... that strikes me as really ugly ... I am still thinking about what to do here. I agree some error handling is warranted.

While I am listing differences ... d7 is using span elements as wrapper, wrapped inside the node-links ... The patch currently uses divs Hmm..

Anyway I will post a patch Sunday....

A little more on this issue...

from #50

so no need to copy over the use of the flag-waiting class.

err my bad, flag-waiting should be copied as it styles the links during the time which the XHR is in flight.

some additional comments for the next iteration.

    if (status === 'success') {
      // Construct a <p> element with escaped message.
      var p_elem = $('<p class="js-flag-message">').text(response.message);

      $(response.selector).append(p_elem);

      p_elem.fadeIn('fast', function () {
        $(this).fadeOut(3000, function () {
          $(this).remove();
        });
      });
    } else {
      // If the XHR failed, assume that the replace command which would normally make
      // the styling disappear has failed and remove the temporary styling.
      $('flag-waiting').toggleClass('flag-waiting');
    };

1) Added the justification, regarding error handling, from #52
2) When adding a new flag, we now default to AJAX action links

reroll. Next step is unit testing.

Err I mean next step JavascriptFunctionalTest, my bad.

A) Fixed up the tests from bad reroll.

B) Its is just the first draft of the test.

1) It is not fully working yet, but the basic structure is there.
2) What the test does not currently do is check for the removal of the flashed messages.
To my eyes Drupal core and mink are missing some functionality.
I want the opposite of waitForElementVisible() which will wait for a mink node element to be removed from the DOM
suggestions welcome

Removed debug, which must look to testbot as a hacking attempt ... soz.

Reroll after recent commit spree.

Hi @kedramon

I think we should hold off on the reroll for a short period of time....

Here is my chain of thought

1) This issue needs to be reworked in light of (#39) The styling is ... broken/unexpected.
2) That means a refactor to square up the handling of contextual links.
3) Before that I want to get our contextual links tests converted/modernised..
4) That way when we come to adding extra tests here .. we have a solid pattern to start from

So I am postponing on the issue listed.

This is stuck behind two issues.... which I have advanced as far as I can ...

I am unassigning - so I can signal the focus of my attention is moving to things I can shepard forward.

Both issues are now clear... I will try and update our shiny new contextual links tests with AJAX specific tests... sometime this week.

Just adding a TODO item here:-

#2886308: "Unflag not allowed text" does not work

is coming down the pipe -- and in this issue we will need to handle the case that AJAX link may be rejected with flag/unflag is not allowed type responses.

Reroll.

Just making notes, this was committed today.

#2764931: Contextual links don't work with 'use-ajax' links

alters the binding of 'use-ajax' elements in ajax.es6.js.... I need to go back and see how this changes things.... for this issue.

There is an aspect of the design I want to cut out....

Modern best practice is to remove jQuery where possible ( in our case if we are the only thing on the page requiring jQuery then there is a 85Kb penalty added to the page weight ,, just to service our little 'ole link )

So .fadeIn and fadeOut are removed. as well as all $() function.

To summarize the code flow.

When the message is added to the DOM ... a css animation is started ... when the animation is finished the message DOM element removes itself.

Every article I read about animations says it is the prefered way as browsers can readily push such effects onto the graphics card and can result in a less janky experience ... that is not something I can see myself in manual testing but maybe on a marginal phone .... a small number of users will get a better experience.

I wanted to do the refactor in es6 ... so I took bold step - which I may unwind...as the language structures I ended up using ... maybe do not justify the change.

I copied over all the script from D8 core and included all the node and yarn files so that we can generate humble javascript from es6 code.

D8 has produced a primer on getting started with es6

https://www.drupal.org/node/2815083

in short once configured ... to develop es6 files you need this process running from the command line

yarn run watch:js

I appreciate my efforts elsewhere to start using es6 have fallen flat ... so I understand if this is contentious and out of scope...

#2880014: Start using es6?

Just making notes for myself and the next iteration

There is talk here of dropping support for -ms- Vendor Prefixes for IE10.

https://www.impressivewebs.com/dropping-ms-vendor-prefixes-ie10/

which is dated 2012

Here is a quote related to 8.4.x from

https://www.drupal.org/docs/7/theming/tools-and-best-practices/recommend...

we will be dropping support for Internet Explorer 9 and 10. We will continue to support IE 11 and Microsoft Edge

Long story short I am going to drop -ms-animation etc ....

1) Dropped vendor prefixes as per #73

2) Copied over core's node lint definitions .. applied core's/air-b-n's coding standard to the new javascript.

This reduced the line count of the javascript significantly.
Refactored the foreach loops.

3) Changed the quadratic bezier used to define the animation from "ease-out" to "ease-in-out". As that feels more visually pleasing.

I have a definitive answer to the test failure .. I do not yet have a solution. so suggestions welcome ;)

Let me start by saying everything works fine when I manually test AjaxLinkTest

The test harness the combination phantomjs, mink etc, is not executing the javascript in flag-action_link_flag.js

The javascript loaded as a consequence of adding 'use-ajax' is being loaded and executed.

Here is the test that demonstrates that to me.

When I alter the javascript to display the flashed message but remove the callback which deletes the p element
Then when I manually test I see wait icon triggered by the use of the 'use-ajax' string and then the message slowly appears but is not removed.

Under these conditions, in the test itself if I take two screenshots - screenshot A immediately after clicking on the ajax action flag link and then take screenshot B immediately after waiting for the message to become visible

Then I see the wait icon in A, and in B the wait icon has gone, but the flashed message is not visible.

[ see command takes a screenshot

$this->createScreenshot('PATH/TO/screenshot.jpg'); ]

Has anyone familiar with both functionalJavascript tests and it limitations see anything else like this before
[ I think I have uncovered a D8 core bug ]

Anyway I will try and take a proper look this on Sunday.

When I add the printer switch :-

../vendor/bin/phpunit --printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" --debug -v ../modules/flag/tests/src/FunctionalJavascript/AjaxLinkTest.php

I can load, in a browser, the web page stored during testing where the flag action link is displayed. When I inspect the relevant DOM element I can see the event listener is attached ... so I am unsure why portions of the javascript dot not function. or why the waitForELementVisible is failing.

in minor news I spotted a small bug which I am fixing here.

I have the feeling that the problem is a FunctionalJavascript issue

There is a raft of issues in core related to updating phantomjs/mink etc

All for performance and/or in preparation for php7.2 - nothing that speaks to errors directly ... but I still want to wait until core is more stable.

#2832672: [PP-1] Upgrade jcalderonzumba/* for better test performance
#2929477: Update jcalderonzumba/mink-phantomjs-driver
[https://github.com/jcalderonzumba/MinkPhantomJSDriver/pull/68

Locally tests pass :tada

1) Fair Warning I am employing a Drupal8.6.x feature -- "Danger Danger Will Robinson".

Specifically I am using the selenium driver

so overriding this to be :-

JavascriptTestBase::minkDefaultDriverClass = DrupalSelenium2Driver::class

So now if desired we can see the tests scroll passed in a browser....and by all reports dropping phantomjs is good because it is a bit more rigorous about the tests.

2) As minor update I saw a d8 core issue recently swapped the order of arguments of assertEquals( $expected, $actual ) so that the first value matches what the test expects ... I have corrected flag's new test so that it is also semantically clean.

3) I have removed some double quotes in favour of single quotes.

What can I say ... it works for me .. but I am not sure that Drupal functional testing is stable/repeatable
I think the system of checking in the new test is ok. There are no wait periods inserted to get it to work [ which would be a brittle design ].. but I am still not confident testbot will come back green.

In an effort to fix a unrelated issue in D8 core ... the mink-selenium2-driver has been significantly upgraded.
I think it is worth testing this again just to see if stability in general has been improved.

.. This needed to be rerolled .. nothing complex to fix just auto-merging.

#2944291: Upgrade behat/mink-selenium2-driver to 1.3.x-dev

My response to the failing tests - was to refactor it - in a effort to get stability.

#2944815: Update FlagBookmarkUITest

Now that activity is complete .. this issue needs a reroll .. the new patch is similar -- just changes to FlagBookmarkUITest are removed.

1) This issue introduces a change and makes the default flag, an 'ajax_link' flag.

In this change the flags defined in flag_follower and flag_bookmark also default to 'ajax_link'

2) I have cleaned up a ultra trivial coding standard nit.

3) We now have working tests ... fingers crossed.

Thanks for reviewing and commiting this .. it was not the easiest patch to review.

I think maybe the patch from #83 was committed ... without the changes from #84

the effect is that flag_bookmark and flag_follower default to normal ( non-ajax ) link types.

So I have created a follow up issue.

I maybe I was wrong but back in march I observed this (#50)

included all the HTML elements because it was interesting to note how D7's defaults differ from D8. While I am marking differences ... one thing this issue needs to restore is that AJAX link should be the default as they are in D7.

Regarding flag_bookmark and flag_follower ... I think the change is appropriate 15-20 years ago always clicking on a link and getting new page was acceptable .. now I think the sensible default is ajax_links where possible.

I did not realise it was going to be contentious?

const links = context.querySelectorAll

Seems not to be a IE11 thing ...This stack overflow issue

https://stackoverflow.com/questions/45260298/queryselectorall-method-is-...

@Matroskeen - it would help if you could confirm this is the error in the console output?

Object doesn't support property or method 'querySelector'

I have transferred your update on the error message to the issue where I am looking at the regression.

https://www.drupal.org/project/flag/issues/2948434#comment-12502624

I have a patch pending on that issue.