Closed
Bug 1226904
Opened 9 years ago
Closed 9 years ago
Hit-testing is broken for photos & links-to-photos in Yelp comments. (They don't get styled on hover, and clicking has no effect)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox43 | --- | unaffected |
firefox44 | --- | unaffected |
firefox45 | + | unaffected |
firefox46 | --- | fixed |
People
(Reporter: dholbert, Assigned: sinker)
References
Details
(Keywords: regression)
Attachments
(4 files, 3 obsolete files)
(deleted),
video/ogg
|
Details | |
(deleted),
application/gzip
|
Details | |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
STR:
1. Visit http://www.yelp.com/biz/j-and-m-hobby-house-san-carlos?start=20
2. Scroll down to the comment with some photos from "lynn c." (it's the 4th comment shown on the page, right now)
3. Hover the link below the photos. If you like, try to click it.
EXPECTED RESULTS:
Link & cursor should change on link-hover (as they normally do). Clicking link should be functional.
ACTUAL RESULTS:
Link/cursor do not change on hover. Clicking link has no effect.
This regressed in the last week. m-c regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48d636f678b0e5162ab868dc9024a5ffe350460c&tochange=898c2c656e4b156c323416ef0c859915f3fd2308
Suspecting Bug 1208673, since it's about HitTest and hit-testing seems to be broken here. Will know more definitively after mozregression gets through some inbound builds.
Reporter | ||
Comment 1•9 years ago
|
||
Actually, the photos themselves are also linkified, and the seem to have the same issue. (When hovering the photos, your cursor doesn't change, or changes inconsistently -- and clicking doesn't reliably open a larger view of the photo.)
Summary: Hit-testing is broken for links in Yelp comments. (They don't get styled on hover, and clicking has no effect) → Hit-testing is broken for photos & links-to-photos in Yelp comments. (They don't get styled on hover, and clicking has no effect)
Reporter | ||
Comment 2•9 years ago
|
||
Inbound regression range confirms that this is a regression from Bug 1208673 (or something else in its push; but it's the only hit-testing related thing there).
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22360424ed1532c73c93aab85bfd9173899956f8&tochange=8084077bd124bc25f521fd31fe51a9d1c508c68a
Thinker, could you take a look?
Flags: needinfo?(tlee)
Assignee | ||
Comment 4•9 years ago
|
||
Daniel,
I have tried it with the code pull from m-c today. But, it seems work good, I don't see the issue you had. Link and cursor change their style for hover, and clicks also work. Could you give me a dump of displaylist for |hittest|?
Updated•9 years ago
|
Flags: needinfo?(dholbert)
Reporter | ||
Comment 5•9 years ago
|
||
Odd that you're not seeing it -- I can reproduce reliably, in latest nightly & in an up-to-date debug build, using a different machine from the one I used when filing the bug.
Happens regardless of whether e10s is enabled/disabled, too. Maybe there's some OS dependency here? (I'm on Ubuntu Linux.)
Anyway, I'll look into getting a display-list hittest dump.
Reporter | ||
Comment 6•9 years ago
|
||
Here's a screencast of the bug, showing that my cursor doesn't think it's hovering anything clickable, except for a very small region at the left edge of the link text.
Flags: needinfo?(dholbert)
Reporter | ||
Comment 7•9 years ago
|
||
This page is broken for me, too (and it's a bit easier to see here because you don't have to scroll down to find the broken content):
http://www.yelp.com/biz_photos/j-and-m-hobby-house-san-carlos
In Firefox release, when I hover a photo there, the photo gets a "brighten" effect and my cursor changes to indicate that it's clickable. This doesn't happen in current Nightly.
Reporter | ||
Comment 8•9 years ago
|
||
Not 100% sure what you're looking for, RE "dump of displaylist for |hittest|" -- mattwoodrow suggested I set gDumpEventList to true, so I did that & wiggled the cursor over the should-be-linkified-text-and-images from comment 0 for ~1 second.
Here's a log of my output, from that second. (nearly 2 MB uncompressed, so I gzipped it)
If you still can't reproduce locally (even on the URL from comment 7) and you need a different sort of dump/log, please let me know.
Flags: needinfo?(tlee)
Comment 9•9 years ago
|
||
Bug 1228279 is probably a dupe of the same underlying issue, but it has a different test URL.
Assignee | ||
Comment 10•9 years ago
|
||
Daniel, I get your point. I used to think the problem occurred when be hovering the picture at left side, but I have made out it occurring at the photos in the message content after look into your log. So, I can reproduce it now.
Flags: needinfo?(tlee)
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Daniel, please help me to check if the patch fix the problem here.
Flags: needinfo?(dholbert)
Reporter | ||
Comment 13•9 years ago
|
||
Yup, that patch seems to fix the problem for me, in my local debug build. Thanks!
Flags: needinfo?(dholbert)
Assignee | ||
Comment 14•9 years ago
|
||
--
Attachment #8693307 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8693307 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8693505 [details] [diff] [review]
Fix boundary checking for leaves collecting, v2
Review of attachment 8693505 [details] [diff] [review]:
-----------------------------------------------------------------
Robert, this patch include some minor changes and refactoring works.
::: layout/base/nsDisplayList.cpp
@@ +1887,5 @@
> // For 3d transforms with preserve-3d we add hit frames into the temp list
> // so we can sort them later, otherwise we add them directly to the output list.
> nsTArray<nsIFrame*> *writeFrames = aOutFrames;
> if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM &&
> + static_cast<nsDisplayTransform*>(item)->IsLeafOf3DContext()) {
It did not consider the case of separator items, so that separator items were not handled with right depth. The change of this line would fix the problem.
@@ +5463,4 @@
> // XXX: should go back to fix mTransformGetter.
> if (!mTransformPreserves3DInited) {
> mTransformPreserves3DInited = true;
> + if (!IsLeafOf3DContext()) {
It did not consider the case of separator items, so that local transform were returned while the accumulated one is the correct one for leaves.
Attachment #8693505 -
Flags: review?(roc)
Comment on attachment 8693505 [details] [diff] [review]
Fix boundary checking for leaves collecting, v2
Review of attachment 8693505 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, but definitely needs a test!
Attachment #8693505 -
Flags: review?(roc) → review+
Updated•9 years ago
|
Reporter | ||
Comment 18•9 years ago
|
||
Thinker, do you know when this will be ready to land? (Looks like it's just waiting on an automated test, I think?)
This is causing pretty bad breakage on major sites (particularly the sites on likely-dupe bug 1228279), so it'd be nice to have that breakage fixed to avoid pushing users off of Nightly.
Flags: needinfo?(tlee)
Reporter | ||
Comment 19•9 years ago
|
||
> it'd be nice to have that breakage fixed
> to avoid pushing users off of Nightly.
er, "pushing users off Nightly and Aurora/DevEdition" (this affects both channels).
Assignee | ||
Comment 20•9 years ago
|
||
I will add a test case, and land it in this week.
Flags: needinfo?(tlee)
Assignee | ||
Comment 21•9 years ago
|
||
--
Attachment #8693505 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8693505 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #21)
> Created attachment 8700934 [details] [diff] [review]
> Fix boundary checking for leaves collecting, v3
>
> --
> Attachment #8693505 [details] [diff] [diff] - Attachment is obsolete: true
Add a test case and some minor changes for nsDisplayPerspective.
Assignee | ||
Updated•9 years ago
|
Attachment #8700934 -
Flags: review?(roc)
Attachment #8700934 -
Flags: review?(roc) → review+
Assignee | ||
Comment 23•9 years ago
|
||
--
Attachment #8700934 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8700934 -
Attachment is obsolete: true
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8701316 [details] [diff] [review]
Fix boundary checking for leaves collecting, v4
Robert, I found the mochitest test_preserve3d_sorting_hit_testing.html also hit the problem of bug 1232060. I don't know how it escaped from previous tests. By the way, I add a translate to elements.
Attachment #8701316 -
Flags: review?(roc)
Attachment #8701316 -
Flags: review?(roc) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 26•9 years ago
|
||
Keywords: checkin-needed
Comment 27•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 29•9 years ago
|
||
Given bug 1231004 has been marked as a duplicate I'll just add this here, it seems to have moved into Aurora now.
Comment 30•9 years ago
|
||
Recent regression, tracking.
Thinker, could you fill an uplift request to aurora? Thanks
Flags: needinfo?(tlee)
Reporter | ||
Comment 31•9 years ago
|
||
(Verified fixed in Nightly, using URLs from comment 0 & comment 7, using yesterday's Nightly 46.0a1 (2015-12-28))
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 39•9 years ago
|
||
please check in ff45.
Flags: needinfo?(tlee)
Attachment #8705478 -
Flags: checkin?
Assignee | ||
Comment 40•9 years ago
|
||
Reporter | ||
Comment 41•9 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #39)
> please check in ff45.
You need to tick the "approval‑mozilla‑aurora" flag on the attachment, and answer the questions that appear in the comment box when you tick that request, and wait for approval to be granted by a release manager. (Then someone will check in the patch on your behalf, generally).
(See https://wiki.mozilla.org/Tree_Rules#mozilla-aurora for more.)
Flags: needinfo?(tlee)
Assignee | ||
Comment 42•9 years ago
|
||
Comment on attachment 8705478 [details] [diff] [review]
Fix boundary checking for leaves collecting -- for ff45
Approval Request Comment
[Feature/regressing bug #]: 1226904
[User impact if declined]: hit-test woul fail for elements in preserve-3d context.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cafad6ee9178
[Risks and why]:
[String/UUID change made/needed]:
Flags: needinfo?(tlee)
Attachment #8705478 -
Flags: checkin? → approval-mozilla-aurora?
Comment 43•9 years ago
|
||
This is not a correct uplift request. We need you to fill this "[Risks and why]:"
Thanks.
Flags: needinfo?(tlee)
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8705478 [details] [diff] [review]
Fix boundary checking for leaves collecting -- for ff45
Approval Request Comment
[Feature/regressing bug #]: 1226904
[User impact if declined]: hit-test woul fail for elements in preserve-3d context.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cafad6ee9178
[Risks and why]:
Without this patch, hit-testing would not work correctly with preserve-3d.
[String/UUID change made/needed]:
Flags: needinfo?(tlee)
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8705478 [details] [diff] [review]
Fix boundary checking for leaves collecting -- for ff45
Approval Request Comment
[Feature/regressing bug #]: 1226904
[User impact if declined]: hit-test would fail for elements in preserve-3d context.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cafad6ee9178
[Risks and why]:
No special risk at this moment.
Without this patch, hit-testing would not work correctly with preserve-3d.
[String/UUID change made/needed]:
Comment 46•9 years ago
|
||
Comment on attachment 8705478 [details] [diff] [review]
Fix boundary checking for leaves collecting -- for ff45
Even if the patch is big, taking it because it has tests, impacts potentially of a lot of sites and many duplicates.
Attachment #8705478 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 47•9 years ago
|
||
bugherder uplift |
Comment 48•9 years ago
|
||
This along with the change that caused this regression were backed out from Firefox 45.
https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
Updated•9 years ago
|
Attachment #8705478 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Depends on: CVE-2016-5263
Updated•2 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•