1447993 - after ff update, anchors inside our slider stopped working.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[wesseloh]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

after Firefox update (from 59.x to 59.0.1)

1. go to https://www.kardani.de/ (pwd: cursoriam)
2. click the button (anchor) inside a slider (at the top)



Actual results:

no reaction


Expected results:

navigate to url from href

Comment 1

[wesseloh]

User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: Site compatibility issue. And the bxSlider jQuery library seems wide splayed.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ec28a490365c49cfd4320648652441e1ad8e779f&tochange=2155af8d88c3bd116305378d7e22b947db339c55

Triggered by: Bug 1420589


Very similar to Bug 1446771. Both site used bxSlider jQuery library( https://bxslider.com/ )

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And bxSlider seems to be buggy :/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

thinking if we can emulate Chrome's behavior here

Comment 5

[wesseloh]

why did it work in previous version?

Comment 6

[wesseloh]

I have found out what the cause is:

touch swipe feature is buggy, it prevents buttons to be clicked.
so if you disable touch swiping, buttons start working fine.

use this option:

    touchEnabled: false // disabled due to a bug in bxslider's touch events in Firefox

Comment 7

[wesseloh]

Important: This option has been introduced to our bxslider, so this bug is not reproducible on our page anymore!

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

this is a bug in bxslider.
https://github.com/stevenwanderski/bxslider-4/issues/1188
It has been using pointerId values in a wrong way.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

As far as I know this is not a bug in Firefox. However it is possible, since bxslider seems to be used by many websites, that we need to somehow try to use similar pointerId values as Chrome in order to
workaround the bug in bxslider.

I haven't yet figured out how to do that workaround (and I need to get a pen device to test pen input too)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointer_capture_click_hack.diff

This is bringing in the horrible click event hack Chrome has.
The target of click doesn't need to be in the mouseup path at all.

I need to clean this up and test some more, since the behavior is really odd in Chrome.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ee0acb30ad52b5eb0ace6d8316c1448c25ce63
remote: recorded changegroup in replication log in 0.089s

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: override_click_target.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/f9ef20adcbe042cbfea23dbbbfb691aa8407a393
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ef20adcbe042cbfea23dbbbfb691aa8407a393
remote: recorded changegroup in replication log in 0.097s

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: override_click_target_2.diff

minor fix for debug builds.
Seems to still work ok on touchscreen too.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: override_click_target_2_tests.diff

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea01111481547421d245a742862c73cf685a7dd
remote: recorded changegroup in replication log in 0.083s

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963104 [details] [diff] [review]
override_click_target_2.diff

This is pretty horrible. We need to hit test on *up even if the pointer was captured, and we pass that hit test target to ESM so that click event can be dispatched.
The actual change isn't that complicated - most of the patch is just passing that override click target around.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963290 [details] [diff] [review]
override_click_target_2_tests.diff

This tries to test couple of different variants.
The iframe case especially is interesting, that is something where Chrome is totally broken. Edge works fine.
The test functions are a bit repetitive, but it was simpler to keep those
functions kind of individual unit tests, except the last one which just tests that this stuff works also when capturing target is display:none; (PresShell code path for that case is different.)

But as such, the tests just ensure the right event path.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8963104 [details] [diff] [review]
override_click_target_2.diff

Review of attachment 8963104 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/EventStateManager.cpp
@@ +931,5 @@
> +    nsIPresShell* shell = aContent->OwnerDoc()->GetShell();
> +    if (shell) {
> +      nsPresContext* prescontext = shell->GetPresContext();
> +      if (prescontext) {
> +        RefPtr<EventStateManager> esm = prescontext->EventStateManager();

nsPresContext::EventStateManager() returns EventStateManager*. So, I don't think that this method should return add-refed EventStateManager since new callers of this method may only need raw point instead.

::: dom/events/EventStateManager.h
@@ +97,5 @@
>                            WidgetEvent* aEvent,
>                            nsIFrame* aTargetFrame,
>                            nsIContent* aTargetContent,
> +                          nsEventStatus* aStatus,
> +                          nsIContent* aOverrideClickTarget);

Could you add explanation of this argument including this is nullable.

::: layout/base/PresShell.cpp
@@ +7056,5 @@
> +          // If we can't process event for the capturing content, release
> +          // the capture.
> +          PointerEventHandler::ReleaseIfCaptureByDescendant(
> +            pointerCapturingContent);
> +          return NS_OK;

Don't we need to dispatch lostpointercapture event to the pointerCapturingContent?
https://w3c.github.io/pointerevents/#the-lostpointercapture-event

@@ +7230,5 @@
> +          return NS_OK;
> +        }
> +
> +        targetFrame = pointerCapturingContent->GetPrimaryFrame();
> +        frame = targetFrame;

Not scope of this bug though, for what of |frame| is really unclear and I need time to check *where* is this chunk in PresShell::HandleEvent() during this review. So, perhaps, we should separate/reorganize PresShell::HandleEvent() and related methods later.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8963290 [details] [diff] [review]
override_click_target_2_tests.diff

Review of attachment 8963290 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/test/window_bug1447993.html
@@ +39,5 @@
> +      }
> +    }
> +
> +    function start() {
> +      opener.ok(true);

I was sometimes confused with ok/is log without explicit message. Could you append "starting tests..." or something?

@@ +86,5 @@
> +
> +      synthesizeMouseAtCenter(target, {}, window);
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");

Please insert the function name, topLevelDocumentEventHandling, because it's unclear which test is failed from the log.

@@ +88,5 @@
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);

Please append message something like `topLevelDocumentEventHandling: unexpected event at ${i}`.

@@ +90,5 @@
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);
> +        for (var j = 0; j < eventLog[i][1].length; ++j) {
> +          opener.is(eventLog[i][1][j], expectedEvents[i][1][j]);

Please append message something like `topLevelDocumentEventHandling: unexpected composed path to the event target path at ${j} of ${i}`.

@@ +96,5 @@
> +      }
> +      next();
> +    }
> +
> +    function topLevelDocumentEventHandling() {

Exactly same function as above??

@@ +166,5 @@
> +        ["pointerup", [ area, body, html, document, window ]],
> +        ["touchend", [ target, area, body, html, document, window ]],
> +        /*
> +        // Right now touch event initated mousedown/up (and then click) are
> +        // dispatched in APZ, and there isn't JS exposed way to test that.

Oh, I don't know why this...

@@ +205,5 @@
> +
> +      synthesizeTouchAtCenter(target, {}, window);
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");

So, please include the function name.

@@ +207,5 @@
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);

Please append similar message like the previous test.

@@ +209,5 @@
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);
> +        for (var j = 0; j < eventLog[i][1].length; ++j) {
> +          opener.is(eventLog[i][1][j], expectedEvents[i][1][j]);

ditto.

@@ +266,5 @@
> +
> +      synthesizeMouseAtCenter(target, {}, win);
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");

Same as previous test, please insert the function name.

@@ +268,5 @@
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);

Same as previous test.

@@ +270,5 @@
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);
> +        for (var j = 0; j < eventLog[i][1].length; ++j) {
> +          opener.is(eventLog[i][1][j], expectedEvents[i][1][j]);

And here.

@@ +276,5 @@
> +      }
> +      next();
> +    }
> +
> +    function iframeEventHandlingWithCapturingInParent() {

Wow, tricky case... Thank you for adding this kind of test!

@@ +330,5 @@
> +
> +      synthesizeMouseAtCenter(target, {}, win);
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");

Insert the function name, but this is called by iframeEventHandlingWithHiddenCapturingInParent() too... Perhaps, needs to set temporary variable to function name deciding with document.getElementById("targetOutsideIframe").style.display's value.

@@ +332,5 @@
> +
> +      opener.is(eventLog.length, expectedEvents.length,
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);

And here.

@@ +334,5 @@
> +                "Same number events expected.");
> +      for (var i = 0; i < eventLog.length; ++i) {
> +        opener.is(eventLog[i][0], expectedEvents[i][0]);
> +        for (var j = 0; j < eventLog[i][1].length; ++j) {
> +          opener.is(eventLog[i][1][j], expectedEvents[i][1][j]);

And here.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> ::: dom/events/EventStateManager.cpp
> @@ +931,5 @@
> > +    nsIPresShell* shell = aContent->OwnerDoc()->GetShell();
> > +    if (shell) {
> > +      nsPresContext* prescontext = shell->GetPresContext();
> > +      if (prescontext) {
> > +        RefPtr<EventStateManager> esm = prescontext->EventStateManager();
> 
> nsPresContext::EventStateManager() returns EventStateManager*. So, I don't
> think that this method should return add-refed EventStateManager since new
> callers of this method may only need raw point instead.
Nothing keeps the ESM alive then. We do need strong pointer, and I want to ensure that.


> @@ +97,5 @@
> >                            WidgetEvent* aEvent,
> >                            nsIFrame* aTargetFrame,
> >                            nsIContent* aTargetContent,
> > +                          nsEventStatus* aStatus,
> > +                          nsIContent* aOverrideClickTarget);
> 
> Could you add explanation of this argument including this is nullable.
ok


> ::: layout/base/PresShell.cpp
> @@ +7056,5 @@
> > +          // If we can't process event for the capturing content, release
> > +          // the capture.
> > +          PointerEventHandler::ReleaseIfCaptureByDescendant(
> > +            pointerCapturingContent);
> > +          return NS_OK;
> 
> Don't we need to dispatch lostpointercapture event to the
> pointerCapturingContent?
> https://w3c.github.io/pointerevents/#the-lostpointercapture-event
Well, sure. I was just adding the release in order to ensure we don't keep the capture forever.
I'll file a followup, since we don't current release nor fire lostpointercapture, we just return early.


> Not scope of this bug though, for what of |frame| is really unclear and I
> need time to check *where* is this chunk in PresShell::HandleEvent() during
> this review. So, perhaps, we should separate/reorganize
> PresShell::HandleEvent() and related methods later.
Totally agree. I have to always read the method several times to remember what variable means what in each context.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: override_click_target_3.diff

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: override_click_target_3_tests.diff

Added message to each test assertion and removed that embarrassing copy-paste error with duplicate method.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8712aa41c6a
when handling pointerup while there is pointercapture, do a hit test in order to find the click target, r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d82cc09359
when handling pointerup while there is pointercapture, do a hit test in order to find the click target, tests, r=masayuki

Comment 23

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b8712aa41c6a
https://hg.mozilla.org/mozilla-central/rev/19d82cc09359

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963636 [details] [diff] [review]
override_click_target_3.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 1411467
Note, this regression shows a bug in Chrome and Edge, and the plan is to eventually revert the change once those browsers have implemented the correct behavior.
[User impact if declined]: Some web pages not working the way they used to
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: by test
[Needs manual test from QE? If yes, steps to reproduce]: NA 
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: It is a bit risky
[Why is the change risky/not risky?]: we're changing a new feature
[String changes made/needed]: NA

Comment 25

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 8963636 [details] [diff] [review]
override_click_target_3.diff

Restore parity with Chrome and Edge until they're able to fix their buggy implementations. Approved for 60.0b11.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test for beta

Just minor rebasing

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch for beta

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/36be32ab69d2
https://hg.mozilla.org/releases/mozilla-beta/rev/ed5444c44e74

Comment 29

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Olli's assessment on manual testing needs (Comment 24) and the fact that this fix has automated tests.