Closed
Bug 1244221
Opened 8 years ago
Closed 8 years ago
"Restore previous session" button in about:home always wraps
Categories
(Firefox :: General, defect)
Tracking
()
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
410.94 KB,
image/png
|
Details | |
3.03 KB,
patch
|
Felipe
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #962249 +++ Previously, the button to restore the previous session in about:home would show in the same row as the bottom buttons, and only wrap if the window shrinks too much. Now it's always wrapped. STR: - have the setting General -> When Nightly starts -> Show my home page - have some tabs open and restart Nightly
Comment 1•8 years ago
|
||
This affects Windows 7 too.
Assignee | ||
Comment 2•8 years ago
|
||
[Tracking Requested - why for this release]: mozregression pointed to bug 962249 as having caused this, which appears to have landed in 44.
status-firefox44:
--- → affected
status-firefox45:
--- → affected
tracking-firefox45:
--- → ?
tracking-firefox46:
--- → ?
Comment 4•8 years ago
|
||
So about:home page cannot access chrome-only attributes? If that's the case, we should just check window.scrollMaxX and not window.scrollMinX, since the latter is currently chrome-only.
Flags: needinfo?(quanxunzhen)
Comment 5•8 years ago
|
||
And I wonder whether this page could be RTL? If it could, only checking window.scrollMaxX is wrong with RTL. If it is never RTL, then back to checking window.scrollMaxX > 0 works.
Flags: needinfo?(felipc)
Comment 6•8 years ago
|
||
The code affected is here: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/base/content/abouthome/aboutHome.js#372
Assignee | ||
Comment 7•8 years ago
|
||
I believe that this page can indeed be RTL. What would be the right check to replace in this case? I don't understand what's the change in behavior in scrollMaxX
Flags: needinfo?(felipc)
Comment 8•8 years ago
|
||
The behavior of scrollMaxX wasn't changed. What actually regress this is the part 3 of bug 962249 which massively replaced scrollMaxX usage to compare against scrollMinX. The previous behavior was always wrong for RTL (the page can never be narrow because scrollMaxX is always 0 on RTL no matter whether it has scroller). But the new behavior is broken even on LTR because the new scrollMinX is not available in this page. If we want to just back to the previous behavior, we can just check scrollMaxX. But I wonder why scrollMinX is not accessible here. Isn't it loaded in chrome priviledge?
Flags: needinfo?(felipc)
Comment 10•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) (less responsive until Feb 22) from comment #8) > But I wonder why scrollMinX is not accessible here. Isn't it loaded in > chrome priviledge? no, about:home is unpriviledged.
Comment 11•8 years ago
|
||
OK, then we two choices: 1. restore to previous state which was wrong for RTL 2. find some other way to handle this issue, e.g. compare the rect of root against the inner width of window
Flags: needinfo?(felipc)
Comment 13•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) (less responsive until Feb 22) from comment #11) > 2. find some other way to handle this issue, e.g. compare the rect of root > against the inner width of window This seems the most sensible option. I don't know how hard this is to implement correctly though, because it's not clear to me from comment #11 exactly which two things we would be comparing, and some of these measurements round to whole pixels (client/innerHeight/Width) and some don't (getBoundingClientRect and friends).
Flags: needinfo?(gijskruitbosch+bugs)
Comment 14•8 years ago
|
||
Also, I think we should have tests that prevent this regressing again.
Comment 15•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #14) > Also, I think we should have tests that prevent this regressing again. FWIW, mozscreenshots would have definitely caught this… I agree a mochitest would also be good though.
Assignee | ||
Comment 16•8 years ago
|
||
Maybe the best course of action here would be to revert to the previous check which had always been that way, and file a separate bug to figure out the best approach for RTL
Comment 17•8 years ago
|
||
This is my proposed patch for fixing this. I'm not very familiar with tests around this and I'm not quite available recently, so I left that undone, and just requested for a feedback. If anyone could write a test for this issue, that would be appreciated.
Attachment #8715104 -
Flags: feedback?(felipc)
Updated•8 years ago
|
tracking-firefox47:
--- → +
Assignee | ||
Updated•8 years ago
|
Attachment #8715104 -
Flags: feedback?(felipc) → feedback+
Assignee | ||
Comment 19•8 years ago
|
||
Neil: Xidorn wrote the patch, I wrote the test. Please review both parts. I'm unsure about how to guarantee that the size used in the the resizeTo call is good..
Attachment #8717516 -
Flags: review?(enndeakin)
Updated•8 years ago
|
Attachment #8717516 -
Flags: review?(enndeakin) → review+
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8602949a29dc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 22•8 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: regression by bug 962249 [User impact if declined]: about:home looks really ugly (see screenshot) [Describe test coverage new/current, TreeHerder]: patch includes regression test [Risks and why]: small patch, only contained to the position of the "Restore previous session" button of about:home [String/UUID change made/needed]: none
Attachment #8715104 -
Attachment is obsolete: true
Attachment #8717516 -
Attachment is obsolete: true
Attachment #8718339 -
Flags: review+
Attachment #8718339 -
Flags: approval-mozilla-beta?
Attachment #8718339 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Comment 23•8 years ago
|
||
Comment on attachment 8718339 [details] [diff] [review] patch as landed (test improved), r=Enn Have tests, visual regression, taking it. Should be in 45 beta 5.
Attachment #8718339 -
Flags: approval-mozilla-beta?
Attachment #8718339 -
Flags: approval-mozilla-beta+
Attachment #8718339 -
Flags: approval-mozilla-aurora?
Attachment #8718339 -
Flags: approval-mozilla-aurora+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8eb0ec626383
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/30cfcf54f286
Backed out on aurora and beta for test failures: https://hg.mozilla.org/releases/mozilla-beta/rev/bf3681b5be9a https://hg.mozilla.org/releases/mozilla-aurora/rev/9a75286c2b19 https://treeherder.mozilla.org/logviewer.html#?job_id=796287&repo=mozilla-beta
This depends on features added in bug 1245723.
Depends on: 1245723
Assignee | ||
Comment 28•8 years ago
|
||
The test used a helper function that was just present on mozilla-central. I copied that function, tested locally, and relanded: https://hg.mozilla.org/releases/mozilla-aurora/rev/d66138039844 https://hg.mozilla.org/releases/mozilla-beta/rev/b3c3d3dbcca5
Comment 29•8 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #27) > This depends on features added in bug 1245723. So basically we also need fix for bug 1245723 uplifted to Beta for the fix to show in 45, right?
Updated•8 years ago
|
Component: XUL Widgets → General
Product: Toolkit → Firefox
Target Milestone: mozilla47 → ---
Updated•8 years ago
|
Target Milestone: --- → Firefox 47
Assignee | ||
Comment 30•8 years ago
|
||
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #29) > (In reply to Wes Kocher (:KWierso) from comment #27) > > This depends on features added in bug 1245723. > > So basically we also need fix for bug 1245723 uplifted to Beta for the fix > to show in 45, right? No, that was just the test file using a helper function added on bug 1245723. I copied that function to remove the dependency and relanded it.
Comment 31•8 years ago
|
||
Asking because I can see the fix on latest Nightly 47, but not in 45 Beta 5 (Windows 7 x64), and it seems to me like it should be there... or should it be only in Beta 6?
Comment 32•8 years ago
|
||
[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected
Comment 33•8 years ago
|
||
I have reproduced this bug on Nightly 47.0a1 (2016-01-29) on ubuntu 14.04 LTS, 32 bit! The bug's fix is now verified on Latest Nightly 48.0a1! Build ID: 20160412050029 User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [bugday-20160413]
status-firefox48:
--- → verified
Updated•8 years ago
|
Version: unspecified → 44 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•