Closed Bug 1226904 Opened 4 years ago Closed 4 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)

defect
Not set

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)

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.
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)
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)
yes
Assignee: nobody → tlee
Flags: needinfo?(tlee)
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|?
Flags: needinfo?(dholbert)
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.
Attached video screencast of bug
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)
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.
Attached file gzipped event log
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)
Bug 1228279 is probably a dupe of the same underlying issue, but it has a different test URL.
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)
Daniel, please help me to check if the patch fix the problem here.
Flags: needinfo?(dholbert)
Yup, that patch seems to fix the problem for me, in my local debug build. Thanks!
Flags: needinfo?(dholbert)
--
Attachment #8693307 [details] [diff] - Attachment is obsolete: true
Attachment #8693307 - Attachment is obsolete: true
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+
Duplicate of this bug: 1231207
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)
> 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).
I will add a test case, and land it in this week.
Flags: needinfo?(tlee)
--
Attachment #8693505 [details] [diff] - Attachment is obsolete: true
Attachment #8693505 - Attachment is obsolete: true
(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.
Attachment #8700934 - Flags: review?(roc)
Attachment #8700934 - Attachment is obsolete: true
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)
Keywords: checkin-needed
Blocks: 1230072
https://hg.mozilla.org/mozilla-central/rev/f950b7a04741
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1231004
Given bug 1231004 has been marked as a duplicate I'll just add this here, it seems to have moved into Aurora now.
Recent regression, tracking.

Thinker, could you fill an uplift request to aurora? Thanks
Flags: needinfo?(tlee)
(Verified fixed in Nightly, using URLs from comment 0 & comment 7, using yesterday's Nightly 46.0a1 (2015-12-28))
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1235787
Depends on: 1235979
Blocks: 1234768
Blocks: 1236228
Duplicate of this bug: 1236228
No longer blocks: 1236228
No longer blocks: 1228279
Duplicate of this bug: 1228279
Duplicate of this bug: 1232104
Duplicate of this bug: 1232232
Duplicate of this bug: 1234643
Duplicate of this bug: 1235958
No longer blocks: 1230072
please check in ff45.
Flags: needinfo?(tlee)
Attachment #8705478 - Flags: checkin?
(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)
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?
This is not a correct uplift request. We need you to fill this "[Risks and why]:"
Thanks.
Flags: needinfo?(tlee)
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)
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 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+
Blocks: 1240783
Depends on: 1241394
This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
Depends on: 1241779
Attachment #8705478 - Flags: approval-mozilla-aurora+
Depends on: 1246622
Depends on: 1258884
Depends on: 1291826
Depends on: 1517388
You need to log in before you can comment on or make changes to this bug.