Closed Bug 1167616 Opened 9 years ago Closed 9 years ago

Change Search Settings panel is sized incorrectly on new tab page

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED WONTFIX
Firefox 41
Iteration:
41.3 - Jun 29
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 --- wontfix
firefox40 --- verified
firefox41 --- fixed
firefox42 --- unaffected
firefox43 --- unaffected
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: mconley, Assigned: ursula)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Bugday-20150624])

Attachments

(3 files, 6 obsolete files)

40.54 KB, image/png
Details
308.82 KB, image/png
Details
1.21 KB, patch
ursula
: review+
Details | Diff | Splinter Review
STR:

1) Go to about:newtab
2) Click on the magnifying glass icon to the left of the search input

ER:

A panel should open with UI to send the user to the Search Settings preferences.

AR:

A panel opens with the UI, but the UI is kind of cut off. See screenshot.
I also see this on Windows 7 & 8. 
It looks to have been introduced with Firefox 39. Firefox 38.0.5 and earlier don't have the bug.
It's also worse on Nightly & Aurora as compared to Beta 39.
Flags: firefox-backlog+
emtwo, if this is regressed on Beta 39, then bug 1158859 probably didn't regress it? Or maybe only made it worse if it is indeed 1158859?
Flags: needinfo?(msamuel)
Blocks: 1140185
From bug 1172300 attachment 8616431 [details], it looks like there is an okay amount of padding in 39. And attachment 8609589 [details] shows a smaller but not-incorrect amount of padding in 39, so there might have been an tiles-unrelated change from 38 -> 39. But I would guess bug 1158859 changed some menu sizing causing the bad croppiness in 40.
Attachment #8616755 - Flags: review?(vdjeric)
Attachment #8616755 - Flags: review?(msamuel)
Attachment #8616755 - Flags: review?(mconley)
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

I think :emtwo is the right choice here.
Attachment #8616755 - Flags: review?(vdjeric)
Attachment #8616755 - Flags: review?(mconley)
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

> .newtab-customize-panel-item,
> .newtab-search-panel-engine,
> #newtab-search-manage {
>-  padding: 10px 10px 10px 25px;
>+  line-height: 54px;
>+  padding: 3px 10px;
Are there other patches applied?

m-c has different context:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.css#498
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

This patch is for beta and aurora. The patch for Bug 116764 includes the fix for the panel and is for central
Bug 1167643 ** Sorry
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

This patch is for beta and aurora. The patch for Bug 1167643 includes the fix for the panel and is for central
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

Try push link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2a92053bbdd
Attachment #8616755 - Attachment description: Change Search Settings panel is sized incorrectly on new tab page → Change Search Settings panel is sized incorrectly on new tab page for beta/aurora
Ed, the regression began in bug 1126186.

Ursula, I would recommend undoing the changes made in the diff here: https://bugzilla.mozilla.org/attachment.cgi?id=8578967&action=diff

We can return to "padding: 4px 24px;" only for .newtab-search-panel-engine and #newtab-search-manage and keep the current styling just for .newtab-customize-panel-item.

Thanks!
Flags: needinfo?(msamuel)
Comment on attachment 8616755 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

Review of attachment 8616755 [details] [diff] [review]:
-----------------------------------------------------------------

Please refer to comment 13.
Attachment #8616755 - Flags: review?(msamuel)
Blocks: 1126186
Attachment #8616755 - Attachment is obsolete: true
Attachment #8616902 - Flags: review?(msamuel)
Attachment #8616902 - Flags: review?(msamuel) → review+
Assignee: nobody → ursulasarracini
Comment on attachment 8616902 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

Approval Request Comment
[Feature/regressing bug #]:
Bug 1126186

[User impact if declined]:
Panel UI will look very awful (see screenshot)

[Describe test coverage new/current, TreeHerder]:
Manual testing

[Risks and why]: 
Low risk because it's CSS styling

[String/UUID change made/needed]:
None
Attachment #8616902 - Attachment description: Change Search Settings panel is sized incorrectly on new tab page → Change Search Settings panel is sized incorrectly on new tab page for beta/aurora
Attachment #8616902 - Flags: approval-mozilla-beta?
Attachment #8616902 - Flags: approval-mozilla-aurora?
Iteration: --- → 41.3 - Jun 29
Points: --- → 3
Whiteboard: .?
I see that it's a minor change, but since this isn't so noticeable on beta as it is in nightly and aurora, I'd like to uplift this to 40 and let it ride the trains there.
Comment on attachment 8616902 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for beta/aurora

Approved for uplift to aurora. Noticeable regression for users, small css fix.
Attachment #8616902 - Flags: approval-mozilla-beta?
Attachment #8616902 - Flags: approval-mozilla-beta-
Attachment #8616902 - Flags: approval-mozilla-aurora?
Attachment #8616902 - Flags: approval-mozilla-aurora+
This doesn't apply to Aurora.
Whiteboard: .?
According to nhnt11 in bug 1173543, this doesn't need to get fixed on central as the panel itself is getting torn out.

We will, however, need a fix for Aurora.
Attachment #8616902 - Attachment is obsolete: true
Attachment #8620978 - Attachment description: Change Search Settings panel is sized incorrectly on new tab page → Change Search Settings panel is sized incorrectly on new tab page (for aurora)
Attachment #8620978 - Flags: review?(msamuel)
Whiteboard: .?
Comment on attachment 8620978 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page (for aurora)

Review of attachment 8620978 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/newTab.css
@@ +548,1 @@
>  .newtab-search-panel-engine,

We want to still keep .newtab-customize-panel-item here so that we don't change its other properties (line-height, font-size, cursor). But we need to override it's padding (see next comment)

@@ -545,4 @@
>  .newtab-search-panel-engine,
>  #newtab-search-manage {
>    line-height: 54px;
> -  padding-left: 40px;

We need to keep this padding but only for .newtab-customize-panel-item
Attachment #8620978 - Flags: review?(msamuel)
Attachment #8620978 - Attachment is obsolete: true
Attachment #8621146 - Attachment description: Change Search Settings panel is sized incorrectly on new tab page → Change Search Settings panel is sized incorrectly on new tab page (for aurora)
Attachment #8621146 - Flags: review?(msamuel)
Comment on attachment 8621146 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page (for aurora)

Review of attachment 8621146 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good thanks!
Attachment #8621146 - Flags: review?(msamuel) → review+
I assume we won't need another aurora approval request, as this has just been rejigged to apply to aurora, but still fits the same conditions as in comment 17.
Attachment #8616900 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Whiteboard: [good first verify]
I have reproduced this bug in Nightly 41.0a1(Build ID: 20150522030225)(UA: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0) with instruction from comment 0

bug is fixed now on latest Developer Edition 40.0a2 (Build ID: 20150623004006) 

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0

[Bugday-20150624]
Reproduced this bug in Nightly 41.0a1 (2015-05-22) (Build ID: 20150522030225) with instruction from comment 0 and Linux x64.

Verified as fixed with latest Developer Edition 40.0a2 (2015-06-23) 

Build ID: 20150623004006
Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][Bugday-20150624]
Whiteboard: [Bugday-20150624]
A change in padding made moved the "change search settings" text back over in aurora and beta so this bug is re-opened and I'll post a patch to fix the padding again.
Changeset that caused padding change: https://hg.mozilla.org/releases/mozilla-aurora/rev/e7c0ddb5f948
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 1192057
No longer blocks: 1140185, 1126186
Attachment #8621146 - Attachment is obsolete: true
Attachment #8645753 - Flags: review?(msamuel)
Comment on attachment 8645753 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for aurora

Review of attachment 8645753 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/newTab.css
@@ +559,5 @@
>    max-width: 300px;
>  }
>  
> +.newtab-customize-panel-item {
> +  padding-left: 40px;

We need this to be -moz-padding-start so rtl locales get the correct padding too.
Attachment #8645753 - Flags: review?(msamuel) → review+
Attachment #8645753 - Attachment is obsolete: true
Attachment #8645999 - Flags: review+
Keywords: checkin-needed
Target Milestone: Firefox 40 → Firefox 41
I think this is going to need beta uplift approval set on it in order to get checked in.
Keywords: checkin-needed
Hey Ursula - can you request beta approval on this when you get a chance? I think that's the only place where this patch needs to land.
Flags: needinfo?(ursulasarracini)
Comment on attachment 8645999 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for aurora

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Flags: needinfo?(ursulasarracini)
Attachment #8645999 - Flags: approval-mozilla-beta?
Approval Request Comment
[Feature/regressing bug #]: 
Bug 1172987

[User impact if declined]:
UI will not look nice (see original screenshot)

[Describe test coverage new/current, TreeHerder]:
Manual testing

[Risks and why]: 
Low risk since it's just CSS

[String/UUID change made/needed]:
None
Comment on attachment 8645999 [details] [diff] [review]
Change Search Settings panel is sized incorrectly on new tab page for aurora

Approved, the mis-alignment of text looks sloppy and is a regression. Let's uplift to Beta, seems safe and the fix is straightforward too.
Attachment #8645999 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Requesting QE team to verify this is fixed on all the current releases, FF43, FF42, FF41. Thanks!
Flags: qe-verify+
Ursula, next time, please reopen a new bug. Here, it is hard to understand which versions is affected or not and bugs are cheap.

Mike, I think Ursula is in PTO, could you help us by updating the tracking flags? Is it normal that the attachment 8645999 [details] [diff] [review] has no uplift requests to aurora? Thanks
Flags: needinfo?(mconley)
(In reply to Sylvestre Ledru [:sylvestre] from comment #42)
> Mike, I think Ursula is in PTO, could you help us by updating the tracking
> flags? Is it normal that the attachment 8645999 [details] [diff] [review]
> has no uplift requests to aurora? Thanks

Yeah, I can explain what's going on here.

The original bug report affected Nightly 41, Aurora 40 and Beta 39.

In comment 18, lizzard describes how since the problem wasn't as visibly bad on 39 that we should land the patch on 40 and let it ride the trains.

Uplifts then occurred, and 39 was out the door.

Then, on Nightly 42, bug 1171344 landed, which eliminates the panel that this bug refers to.

Then in bug 1172987, a patch was uplifted to Aurora 41 which re-opened this bug (I agree in hindsight that we should have opened a new bug).

Then uplifts occurred.

So the problem affects Beta 41, but does not affect Aurora 42 or Nightly 43, since the panel has been eliminated in the latter two releases.
Flags: needinfo?(mconley)
QA Whiteboard: [good first verify][Bugday-20150624] → [Bugday-20150624]
Closing this bug based on the updated flags and the fact that this landed in beta, which seems to be the only version that this was affecting
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.