Closed Bug 1244221 Opened 4 years ago Closed 4 years ago

"Restore previous session" button in about:home always wraps

Categories

(Firefox :: General, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox44 --- wontfix
firefox45 + fixed
firefox46 + fixed
firefox47 + fixed
firefox48 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

+++ 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.
OS: Mac OS X → All
[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
Flags: needinfo?(quanxunzhen)
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)
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)
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)
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)
Duplicate of this bug: 1244969
(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
Flags: needinfo?(felipc)
Gijs, what do you think?
Flags: needinfo?(gijskruitbosch+bugs)
(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)
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
Attached patch proposed patch (obsolete) — Splinter Review
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+
Attached patch patch with test (obsolete) — Splinter Review
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)
Attachment #8717516 - Flags: review?(enndeakin) → review+
Assignee: nobody → felipc
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8602949a29dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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?
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+
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
Flags: needinfo?(felipc)
(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 → ---
Target Milestone: --- → Firefox 47
(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?
Depends on: 1256165
[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
QA Whiteboard: [bugday-20160413]
Version: unspecified → 44 Branch
You need to log in before you can comment on or make changes to this bug.