Closed Bug 1313753 Opened 4 years ago Closed 3 years ago

Unable to select menu items at http://www.gasciunai.joniskis.lm.lt/

Categories

(Core :: Layout, defect, P3)

49 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox49 --- wontfix
firefox50 - wontfix
firefox51 + wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: miketaylr, Assigned: sinker)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, Whiteboard: [webcompat])

Attachments

(5 files, 4 obsolete files)

Attached image menu.gif
This worked in 48 and doesn't in 49.

STR)

1) go to http://www.gasciunai.joniskis.lm.lt/
2) hover over menu item like "bendruomene", move mouse down when it opens and try to click on sub-menu item

Expected Results:

Can select a sub-menu item

Actual Results:

Menu closes when you move mouse down.

(originally reported on webcompat.com)
mozregression took me here: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=daf8583252b256d279453f6cbd7e8494f1a811c4&tochange=656235d7f868ed8147d65aaa0031760863b343a9

Looks like Bug 1274962 regressed this.

(they've added a note that people should use Chrome instead of Firefox 49)

Probably too late for a fix to land on 50? Dunno.
Blocks: 1274962
Flags: needinfo?(matt.woodrow)
Keywords: regression
[Tracking Requested - why for this release]: Too late to fix this in 50, let's hope we have a fix ready to be uplifted to 51 soon.
Track 51+ as regression in real site.
Hi Astley,
Can you help find an owner for this?
Flags: needinfo?(aschen)
(In reply to Gerry Chang [:gchang] from comment #4)
> Hi Astley,
> Can you help find an owner for this?

I think Mike already ni Matt in comment 1. Let's wait for Matt's feedback before going further.
Flags: needinfo?(aschen)
Priority: -- → P1
Priority: P1 → P3
Hi Jet,
Can you help find an owner for this if Matt is working on other high priority?
Flags: needinfo?(bugs)
Thinker: WDYT?
Flags: needinfo?(bugs) → needinfo?(tlee)
I am looking on this bug.  For now, what I have seen, |HitTest()| stop at a display item out of the preserve-3d items.
Flags: needinfo?(tlee)
Attached file bug-1313753.html
Simplified test case.  The unordered list |<ul>| causes the problem.
The visual overflow area of the 3d-context establisher is not updated for CSS transition.  So, it's size keep staying at origin size.  Once mouse moving out the visual overflow area, it loses hover state.
For the case here, the establisher is |.menuHolder|.
For Contents participate a preserve-3d context, the changes of their overflow areas would not be updated on the establisher of the 3D context later, except they are reflowed.  But, it should be updated.

The transform transitions, in preserve-3d context, here does not cause any reflow.  |HitTest()| would skip preserve-3d sub-tries once pointer/mouse move out their visual overflow areas computed at last reflow, even it is invalid.  It is the reason why the menus would shrink back once the pointer moving down for a certain distance.
Attachment #8821796 - Flags: feedback?(dbaron)
Could you try the patch?
Flags: needinfo?(miket)
Yep, works w/ the patch applied!
Flags: needinfo?(miket)
Thanks~ Mike! Let's move forward. :)
Assignee: nobody → tlee
Status: NEW → ASSIGNED
Flags: needinfo?(matt.woodrow)
[Tracking Requested - why for this release]:
David, any feedback on the attached patch?
Flags: needinfo?(dbaron)
Comment on attachment 8821796 [details] [diff] [review]
Update overflow areas of establisher frames of 3D contexts

Sorry -- should have said this earlier -- but feedback? requests need to include what feedback you want.  I don't know what it is you want to know here.
Flags: needinfo?(dbaron)
Attachment #8821796 - Flags: feedback?(dbaron)
My reaction to the patch, for what it's worth, is that the idea seems reasonable, but I'm suspicious of the fact that it happens at the time of style hint processing (rather than when we *process* entries in the overflow tracker), since it seems like it would miss other places where we add things to the overflow changed tracker.

Perhaps a better fix would be to fix OverflowChangedTracker::Flush so that, at the end, when it propagates the change to the parent, it correctly propagates the change to the correct 3D ancestor that the overflow gets added to (which, if I understand correctly, is the root of the 3-D scene rather than the parent).

That also seems like it would make us do less work for a deep 3-D scene, since we wouldn't end up unnecessarily recomputing overflow for all the frames in between.
Comment on attachment 8821796 [details] [diff] [review]
Update overflow areas of establisher frames of 3D contexts

Based on the previous comment, marking feedback-, in case you were using feedback? to mean "tentative review? but I'm not quite ready to ask for review" (which shouldn't be taken as a default assumption of what it means).
Attachment #8821796 - Flags: feedback-
Please make sure we land a test for this too if possible.
Flags: in-testsuite?
David, Thank you for your feedback.  You suggestion is reasonable for me, but I have one question on it. If overflow is not recomputed for frames in between, will the establisher of the 3D rendering context get correct overflow areas from children since at least one of children are invalid on overflow areas?  I will look into details to clear my question later.

I will go back this bug after Feb 2.
Flags: needinfo?(tlee)
My memory (which might be wrong) is that bug 1274962 (part 6, in particular) changed the computation of overflow areas for preserve-3d scenes so that the element that establishes the scene goes through all of its 3D descendants and gets the overflow area from each, and that each 3D part of the scene doesn't otherwise contribute to overflow areas on its parent.  If that assumption is correct, then the frames in the 3D scene shouldn't need to contribute to the overflow areas of their parent (unless the parent is the root of the 3D scene), but instead only to the root of the 3D scene.  So, given that, I don't think overflow needs to be recomputed for frames in between.

It might be worth double-checking with Matt that he agrees.
Yes, that is correct.
Ok! It is clear now for me.  Thank you for your help!
This patch makes Gecko updating overflow area of the establisher of the 3d rendering context if a frame's overflow area is changed.
Attachment #8821796 - Attachment is obsolete: true
Attachment #8830713 - Flags: review?(dbaron)
Comment on attachment 8830713 [details] [diff] [review]
Update overflow at the establisher of the 3D rendering context

>Bug 1313753 - Update overflow at the establisher of the 3D rendering context.

I'd describe this as "Propagate overflow updates to the establisher of the 3D rendering context rather than the parent"

>+        while (parent &&
>+               parent != mSubtreeRoot &&
>+               parent->Combines3DTransformWithAncestors()) {
>+          // Passing frames in between the frame and the establisher of
>+          // 3D rendering context.
>+          parent = parent->GetParent();

I think here (after the parent->GetParent()) you should assert:

  MOZ_ASSERT(parent, "root frame should never return true for Combines3DTransformWithAncestors");

or something like that.

r=dbaron with that
Attachment #8830713 - Flags: review?(dbaron) → review+
Is this ready to land?
Flags: needinfo?(tlee)
Though a test would still be nice :)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1d9acdc203
Update overflow at the establisher of the 3D rendering context. r=dbaron
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1e1d9acdc203
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Any chance we can land a test for this? Also, please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(tlee)
I would add a test basing on attachment 8821525 [details].
Flags: needinfo?(tlee)
Hi Matt,
I had added a mochitest for this bug.  The idea is to rotate a descendant to get more visible regions at the establisher.  And, send a synthesized |click| event to the new regions to see if it is caught by the establisher.  If visible regions are updated correctly, it would be caught by the establisher |closure|, or it would be caught by the parent |outer|.
Attachment #8837478 - Flags: review?(matt.woodrow)
Give a fixed width to |target| to make sure it is as large as needed.
Attachment #8837478 - Attachment is obsolete: true
Attachment #8837478 - Flags: review?(matt.woodrow)
Attachment #8837940 - Flags: review?(matt.woodrow)
Hi Matt, just like what you mentioned above.  This patch uses elementFromPoint() instead.
Attachment #8837940 - Attachment is obsolete: true
Attachment #8837940 - Flags: review?(matt.woodrow)
Attachment #8838408 - Flags: review?(matt.woodrow)
Attachment #8838408 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8835325 [details] [diff] [review]
Update overflow at the establisher of the 3D rendering context, v2

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]: bug 1274962
[User impact if declined]: Users may can not click on items in a preserve-3d context with transform transition/animation/or changes.
[Is this code covered by automated tests?]: yes, the testcase on the same bug.
[Has the fix been verified in Nightly?]: It was landed in m-c on Feb 18, 2017
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: attachment 8838408 [details] [diff] [review] should be uplifted as its testcase.
[Is the change risky?]: low
[Why is the change risky/not risky?]: It affects only preserve-3d contexts.  And, the change is simple and straight forward.
[String changes made/needed]:
Attachment #8835325 - Flags: approval-mozilla-beta?
Attachment #8835325 - Flags: approval-mozilla-aurora?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28f9476b0dc3
Test case for correctness of visible regions of preserve-3d. r=mattwoodrow
Keywords: checkin-needed
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Tested on Windows 10, Ubuntu 16.04 and Mac OS X 10.10 with FF Nighlty (2017-02-19) and I can confirm the fix, I used the steps from description and this is no longer reproducible.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Comment on attachment 8835325 [details] [diff] [review]
Update overflow at the establisher of the 3D rendering context, v2

verified in nightly, let's take this to aurora and beta, along with the test.
Attachment #8835325 - Flags: approval-mozilla-beta?
Attachment #8835325 - Flags: approval-mozilla-beta+
Attachment #8835325 - Flags: approval-mozilla-aurora?
Attachment #8835325 - Flags: approval-mozilla-aurora+
Depends on: 1343096
Depends on: 1527591
You need to log in before you can comment on or make changes to this bug.