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)
Core
Layout
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)
(deleted),
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
[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.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 1•9 years ago
|
||
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.)
tracking-firefox45:
--- → ?
Comment 2•9 years ago
|
||
Need somebody to look into this regression; pinging Matt first, although other people have clearly been touching relevant code recently.
Flags: needinfo?(matt.woodrow)
Updated•9 years ago
|
status-firefox43:
--- → unaffected
status-firefox44:
--- → unaffected
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Reporter | ||
Comment 7•9 years ago
|
||
[setting ni=mattwoodrow to be sure this is on his to-do list when he returns from PTO next week]
Flags: needinfo?(matt.woodrow)
Reporter | ||
Comment 8•9 years ago
|
||
(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.)
Updated•9 years ago
|
status-firefox46:
--- → affected
tracking-firefox46:
--- → ?
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Comment 12•9 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•