Closed Bug 1230693 Opened 9 years ago Closed 9 years ago

Yelp "Redo search in map" button is un-clickable in Nightly (again)

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 + unaffected
firefox46 + fixed

People

(Reporter: dholbert, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(1 file)

[discovered this bug while trying to verify that bug 1208673 was fixed] STR: 1. Visit any yelp search results page, e.g. this one: http://www.yelp.com/search?find_desc=mozilla&find_loc=San+Francisco%2C+CA&ns=1 2. Click and drag the map. 3. Watch the "Redo search in map" button that appears. Try to click on it. ACTUAL RESULTS: Button cannot be clicked. (When I hover it, the cursor doesn't change to a "finger" to indicate that the button is clickable -- except very briefly & rarely -- and clicks seem to be discarded). EXPECTED RESULTS: Cursor should change to indicate that button is clickable, and button should be clickable. Bug 1208673 covered an issue very similar to this, with this particular button not being clickable [and also having an animation bug]. That bug's patch (landed Nov 16th) fixed that issue -- the button became functional again. But this it broke again on Nov 26th, with this regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ddd566eb39b7a39d0ff630343f179fedc8fcafd5&tochange=7e18014be68d9e13760b2814862745911caf49a5 --> regression from bug 1168263. In current nightlies (Nov 26th up to today's nightly, 2015-12-04) this button is broken.
Requesting tracking, for the reasons described in bug 1208673 comment 4. (popular website, bad user impact if left unfixed, breaks a key use case on the site -- dragging a map around to a different area & updating yelp's search results accordingly.)
Need somebody to look into this regression; pinging Matt first, although other people have clearly been touching relevant code recently.
Flags: needinfo?(matt.woodrow)
We actually have 3 different states for taking the origin into account: * Appending the origin translation * Not appending, but converting the matrix to use the origin as its basis * Ignoring it entirely (when we build an nsDisplayPerspective to handle the translation) https://hg.mozilla.org/mozilla-central/rev/885889d182fd broke the middle case, and made us always use the last one. This adds a new flag so we can differentiate all the use cases. A test would be nice, but I'm struggling to reduce the webpage sufficiently to do so.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Attachment #8699249 - Flags: review?(roc)
Flags: in-testsuite?
Some extra explanation of the changes: In bug 1168263 Comment 54, I showed how the original code was doing: frameToReferenceFrame * -frameToReferenceFrame * -frameToTransformOrigin * transformList * frameToReferenceFrame * frameToTransformOrigin * -frameToReferenceFrame * -frameToPerspectiveOrigin * perspective * frameToReferenceFrame * frameToPerspectiveOrigin Which reduces to: -frameToTransformOrigin * transformList * frameToTransformOrigin * -frameToPerspectiveOrigin * perspective * frameToPerspectiveOrigin * frameToReferenceFrame In the case where we didn't have the initial 'frameToReferenceFrame' (when OFFSET_TO_ORIGIN isn't specified) this instead reduces to: -frameToReferenceFrame * -frameToTransformOrigin * transformList * frameToTransformOrigin * -frameToPerspectiveOrigin * perspective * frameToPerspectiveOrigin * frameToReferenceFrame Which is the same as the normal case, except with the post-translate frameToReferenceFrame removed, and a ChangeBasis(frameToReferenceFrame) instead.
Comment on attachment 8699249 [details] [diff] [review] Rebase to the origin for callers that don't want it offset roc is away apparently, do you mind looking at this Markus?
Attachment #8699249 - Flags: review?(roc) → review?(mstange)
Comment on attachment 8699249 [details] [diff] [review] Rebase to the origin for callers that don't want it offset Review of attachment 8699249 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but a test would be really valuable. Can you attach the reduced testcase that you have, so that I can try to create a test from it?
Attachment #8699249 - Flags: review?(mstange) → review+
[setting ni=mattwoodrow to be sure this is on his to-do list when he returns from PTO next week]
Flags: needinfo?(matt.woodrow)
Blocks: 1234768
(We should backport this to Aurora 45 soon after it's fixed in Nightly, too. Aurora 45 is affected by this bug, and it'll be moving to the beta-channel on Jan 25, and it'd be nice if this fix had some time to bake on Aurora before that point.)
Tracking to make sure it is fixed for 45 (and because it is a regression)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(In reply to Markus Stange [:mstange] from comment #6) > Comment on attachment 8699249 [details] [diff] [review] > Rebase to the origin for callers that don't want it offset > > Review of attachment 8699249 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but a test would be really valuable. Can you attach the reduced > testcase that you have, so that I can try to create a test from it? I can't find it sorry. I didn't get all that far, just saved the yelp page and stripped out most of the page content. Attempting to use the css minimizing tool you suggested broke the test case.
Flags: needinfo?(matt.woodrow)
Uplift to 45?
Flags: needinfo?(matt.woodrow)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Uplift to 45? As mentioned in other bugs, the regressing bug got backed out of 45.
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: