Closed
Bug 1394588
Opened 7 years ago
Closed 7 years ago
Library icon animation is heavily out of position when adding a bookmark
Categories
(Firefox :: Theme, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: Eddwardiq, Assigned: jaws)
References
Details
(Keywords: regression, Whiteboard: [reserve-photon-animation])
Attachments
(3 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170828100127 Steps to reproduce: 1. put Library button on the toolbar (if not yet) 2. open random web page 3. hit star button and click done 4. see broken Library button animation - it shifts down and then up on the navigation toolbar Looks like it started with https://hg.mozilla.org/integration/autoland/rev/b5a5c38a0add So regressed by Bug 1384953. I didn't test animation when saving to Pocket.
Reporter | ||
Updated•7 years ago
|
Blocks: 1384953
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Theme
Keywords: regression
Whiteboard: [photon-animation] [triage]
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
Ever confirmed: true
Comment 1•7 years ago
|
||
As stated in http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows 10 this happens on maximized window without title bar; otherwise the library button animation (i.e. after clicking "Done") is fine.
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P3
QA Contact: stefan.georgiev
Whiteboard: [photon-animation] [triage] → [reserve-photon-animation]
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Mauro Sanabria from comment #1) > As stated in > http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows > 10 this happens on maximized window without title bar; otherwise the library > button animation (i.e. after clicking "Done") is fine. I can not confirm this. Animation works fine only on windowed mode with enabled title bar. In every other case it is broken. The most important thing is that it is broken with default Windows 10 configuration.
Assignee | ||
Comment 3•7 years ago
|
||
In bug 1384953 we use the bounding client rect of the library button icon to position the animatable box for the library animation in the same place. This works well for restored windows, but it is showing this bug in maximized windows. The calculation for the bounding client rect is off by 6.5px in maximized windows, which matches the padding-top on #titlebar for maximized windows. I can't see where this 6.5px comes from, especially because there is no mention of `paddingTop` in http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/base/content/browser-tabsintitlebar.js. To fix this bug I *could* subtract the #titlebar's padding-top from the library icon's bounding client rect Y value, but I would like to understand this bug better. @Dao, do you know where this padding-top is coming from and what I can do to work-around-it or incorporate it into the calc()?
Flags: needinfo?(dao+bmo)
Comment 4•7 years ago
|
||
(In reply to Eddward from comment #2) > (In reply to Mauro Sanabria from comment #1) > > As stated in > > http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows > > 10 this happens on maximized window without title bar; otherwise the library > > button animation (i.e. after clicking "Done") is fine. > > I can not confirm this. > Animation works fine only on windowed mode with enabled title bar. In every > other case it is broken. Are you saying it's also off in windowed mode with the title bar disabled? Because that doesn't match comment #3... If so, can you add a screencast and/or more details about how that case is broken?
Comment 5•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3) > In bug 1384953 we use the bounding client rect of the library button icon to > position the animatable box for the library animation in the same place. > > This works well for restored windows, but it is showing this bug in > maximized windows. > > The calculation for the bounding client rect is off by 6.5px in maximized > windows, which matches the padding-top on #titlebar for maximized windows. I > can't see where this 6.5px comes from, especially because there is no > mention of `paddingTop` in > http://searchfox.org/mozilla-central/rev/ > cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/base/content/browser- > tabsintitlebar.js. > > To fix this bug I *could* subtract the #titlebar's padding-top from the > library icon's bounding client rect Y value, but I would like to understand > this bug better. @Dao, do you know where this padding-top is coming from and > what I can do to work-around-it or incorporate it into the calc()? It's 6.8px for me, and AFAICT it's coming from the -moz-appearance setting, because there are no other rules that show any padding at all. Rather than doing something specific, couldn't you get the lazy bounds of the element against which everything is being positioned (which I think is the navbar?) and subtract the two?
Reporter | ||
Comment 6•7 years ago
|
||
(In reply to :Gijs (queue backed up, slow) from comment #4) > (In reply to Eddward from comment #2) > > (In reply to Mauro Sanabria from comment #1) > > > As stated in > > > http://forums.mozillazine.org/viewtopic.php?p=14763709#p14763709, on Windows > > > 10 this happens on maximized window without title bar; otherwise the library > > > button animation (i.e. after clicking "Done") is fine. > > > > I can not confirm this. > > Animation works fine only on windowed mode with enabled title bar. In every > > other case it is broken. > > Are you saying it's also off in windowed mode with the title bar disabled? > Because that doesn't match comment #3... If so, can you add a screencast > and/or more details about how that case is broken? yes it is, looks like 2px for me, and in maximized window it is 8px as far as I can see... the important thing here is that I'm using compact theme mode... in normal mode it's fine in restored window as per above comments
Reporter | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
(In reply to :Gijs (queue backed up, slow) from comment #5) > > To fix this bug I *could* subtract the #titlebar's padding-top from the > > library icon's bounding client rect Y value, but I would like to understand > > this bug better. @Dao, do you know where this padding-top is coming from and > > what I can do to work-around-it or incorporate it into the calc()? > > It's 6.8px for me, and AFAICT it's coming from the -moz-appearance setting, > because there are no other rules that show any padding at all. Yep, it's coming from -moz-appearance. Are you sure you found the right solution in bug 1384953? This whole setup feels fragile, and this bug seems worse than what bug 1384953 attempted to fix.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 9•7 years ago
|
||
I'm not sure what else we can do if we want to fix bug 1384953. At least with what we know now I can adjust the calculation to take into effect the padding from the #titlebar. I'll update the patch here with that.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8904677 [details] Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation. https://reviewboard.mozilla.org/r/176482/#review181510 ::: commit-message-0e73a:1 (Diff revision 1) > +Bug 1394588 - Include the padding-top of the titlebar in the calcuation of the library drop animation. r?gijs Nit: spelling. ::: browser/base/content/browser-places.js:1534 (Diff revision 1) > - let libraryIcon = document.getAnonymousElementByAttribute(libraryButton, "class", "toolbarbutton-icon"); > + let libraryIcon = document.getAnonymousElementByAttribute(libraryButton, "class", "toolbarbutton-icon"); > + let titlebar = document.getElementById("titlebar"); > - let dwu = window.getInterface(Ci.nsIDOMWindowUtils); > + let dwu = window.getInterface(Ci.nsIDOMWindowUtils); > - let iconBounds = dwu.getBoundsWithoutFlushing(libraryIcon); > + let iconBounds = dwu.getBoundsWithoutFlushing(libraryIcon); > - let libraryBounds = dwu.getBoundsWithoutFlushing(libraryButton); > + let libraryBounds = dwu.getBoundsWithoutFlushing(libraryButton); > + let titlebarPaddingTop = window.getComputedStyle(titlebar).paddingTop; I'm probably missing something, but why couldn't we use the bounds of gNavToolbox, also getting that with dwu? That would also help if other titlebar properties ever get in the way here. Or does the `promiseLayoutFlushed` stuff here fix something else? Making this async, I wonder if we need to check `window.closed` before doing all the actual work in the `requestAnimationFrame` callback.
Attachment #8904677 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904677 [details] Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation. https://reviewboard.mozilla.org/r/176482/#review181510 > I'm probably missing something, but why couldn't we use the bounds of gNavToolbox, also getting that with dwu? That would also help if other titlebar properties ever get in the way here. Or does the `promiseLayoutFlushed` stuff here fix something else? > > Making this async, I wonder if we need to check `window.closed` before doing all the actual work in the `requestAnimationFrame` callback. We can't use the bounds of gNavToolbox because gNavToolbox includes all of the toolbars. We would have to base our animation off of the bottom of gNavToolbox, and then we would have to calculate the height of the bookmarks toolbar and change the way we calculate the positioning. I'm not sure if it would have its own issues. The promiseLayoutFlushed is only here to prevent a sync style flush when calling getComputedStyle. As far as other properties, I agree that this approach isn't perfect but I didn't want to add in extra calculations for paddingBottom, height, etc if they aren't necessary. Good call about window.closed, I'll add that in.
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8904677 [details] Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation. https://reviewboard.mozilla.org/r/176482/#review182398 Clearing this per discussion on IRC. :-)
Attachment #8904677 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8904677 [details] Bug 1394588 - Include the gNavToolbox offset in the calculation of the library drop animation. https://reviewboard.mozilla.org/r/176482/#review182546 Nice and simple. Thanks!
Attachment #8904677 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 18•7 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1dca70dfe391 Include the gNavToolbox offset in the calculation of the library drop animation. r=Gijs
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1dca70dfe391
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 20•7 years ago
|
||
I have reproduced this issue with the Nightly build: 20170905100117 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20170908100218 This bug fix is Verified with latest Nightly 57.0a1. (20170908100218)
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•