Closed Bug 1244221 Opened 4 years ago Closed 4 years ago
"Restore previous session" button in about:home always wraps
+++ 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
This affects Windows 7 too.
[Tracking Requested - why for this release]: mozregression pointed to bug 962249 as having caused this, which appears to have landed in 44.
ni? myself. I'll see what can be done here
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.
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.
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
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?
(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.
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
Gijs, what do you think?
(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).
Also, I think we should have tests that prevent this regressing again.
(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.
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
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)
Attachment #8715104 - Flags: feedback?(felipc) → feedback+
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)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
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
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.
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
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
(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?
Component: XUL Widgets → General
Product: Toolkit → Firefox
Target Milestone: mozilla47 → ---
(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.
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?
[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
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
You need to log in before you can comment on or make changes to this bug.