Closed
Bug 1313753
Opened 8 years ago
Closed 8 years ago
Unable to select menu items at http://www.gasciunai.joniskis.lm.lt/
Categories
(Core :: Layout, defect, P3)
Tracking
()
People
(Reporter: miketaylr, Assigned: sinker)
References
(Depends on 1 open bug, )
Details
(Keywords: regression, Whiteboard: [webcompat])
Attachments
(5 files, 4 obsolete files)
(deleted),
image/gif
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/gif
|
Details | |
(deleted),
patch
|
sinker
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•8 years ago
|
||
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
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
tracking-firefox50:
--- → ?
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.
Comment 5•8 years ago
|
||
(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
Updated•8 years ago
|
Priority: P1 → P3
Comment 6•8 years ago
|
||
Hi Jet,
Can you help find an owner for this if Matt is working on other high priority?
Flags: needinfo?(bugs)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
Simplified test case. The unordered list |<ul>| causes the problem.
Assignee | ||
Comment 10•8 years ago
|
||
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|.
Assignee | ||
Comment 11•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
Thanks~ Mike! Let's move forward. :)
Assignee: nobody → tlee
Status: NEW → ASSIGNED
Flags: needinfo?(matt.woodrow)
Updated•8 years ago
|
status-firefox53:
--- → affected
Updated•8 years ago
|
Comment 15•8 years ago
|
||
[Tracking Requested - why for this release]:
Reporter | ||
Comment 16•8 years ago
|
||
David, any feedback on the attached patch?
Flags: needinfo?(dbaron)
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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-
Updated•8 years ago
|
Flags: needinfo?(tlee)
Comment 20•8 years ago
|
||
Please make sure we land a test for this too if possible.
Flags: in-testsuite?
Assignee | ||
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
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.
Comment 23•8 years ago
|
||
Yes, that is correct.
Assignee | ||
Comment 24•8 years ago
|
||
Ok! It is clear now for me. Thank you for your help!
Assignee | ||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Comment 28•8 years ago
|
||
Though a test would still be nice :)
Assignee | ||
Comment 29•8 years ago
|
||
r=dbaron
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c3c80ba55b06eed80a7d5695ffe23993911b8c
Attachment #8830713 -
Attachment is obsolete: true
Flags: needinfo?(tlee)
Attachment #8835325 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 30•8 years ago
|
||
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
Comment 31•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 32•8 years ago
|
||
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)
Assignee | ||
Comment 33•8 years ago
|
||
I would add a test basing on attachment 8821525 [details].
Flags: needinfo?(tlee)
Assignee | ||
Comment 34•8 years ago
|
||
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)
Assignee | ||
Comment 35•8 years ago
|
||
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)
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cddf3c3c8ff7a2c7331922ff081da9e8a382377e
for the testcase and the code changes rebased to aurora.
Comment 37•8 years ago
|
||
Is there any reason you didn't use elementFromPoint?
http://searchfox.org/mozilla-central/source/layout/base/tests/test_preserve3d_sorting_hit_testing.html#32
Assignee | ||
Comment 38•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8838408 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 39•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8745b0240ebdf0513a01484deeec40df95af1587
please checkin attachment 8838408 [details] [diff] [review]. Thank you!
Keywords: checkin-needed
Assignee | ||
Comment 40•8 years ago
|
||
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?
Comment 41•8 years ago
|
||
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
Comment 42•8 years ago
|
||
bugherder |
Comment 43•8 years ago
|
||
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)
Comment 44•8 years ago
|
||
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.
Comment 45•8 years ago
|
||
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+
Comment 46•8 years ago
|
||
bugherder uplift |
Comment 47•8 years ago
|
||
bugherder uplift |
Comment 48•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/85400a46019b
https://hg.mozilla.org/releases/mozilla-esr52/rev/da123a8b468f
status-firefox-esr52:
--- → fixed
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•