Closed Bug 1205100 Opened 4 years ago Closed 4 years ago

Text in buttons on about:support causes overflow

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 --- verified

People

(Reporter: arni2033, Assigned: Gijs)

References

Details

(Whiteboard: [testday-20151002])

Attachments

(2 files, 1 obsolete file)

Nightly 43, ID 20150915030232
It's very noticeable on HiDPI but also is reproducible with scale=100%. To reproduce this on HiDPI and English locale you may need to reduce the window's width.
Attached patch PatchSplinter Review
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8661744 - Flags: review?(dao)
Attachment #8661744 - Flags: review?(dao) → review+
(In reply to Wes Kocher (:KWierso) from comment #3)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/fb266acf3160
> for crashes:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=4707790&repo=fx-team

WTF. Fancy that for a one-line CSS change. :-\
Offtopic
Actually, if (on Win7) I set HWA -> on, your change causes buttons to have height=31px, while with HWA=off their height=30px (as expected). Is that bug worth reporting or will it be marked Invalid?
Filed bug 1205620 and bug 1205622. Will re-land with a fix to not touch about:addons layout.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #6)
> Filed bug 1205620 and bug 1205622. Will re-land with a fix to not touch
> about:addons layout.

I've changed my mind - this actually breaks a lot of things on OS X, like e.g. about:preferences menulists, which have min-height: 20px !important, which overrides this and therefore they now end up being 10px smaller.

It seems like a much simpler fix will be to fix this specifically in aboutSupport. I don't think we really end up with many in-content buttons with text that long anyway. :-\
Attachment #8662341 - Flags: review?(dao)
Attachment #8661744 - Attachment is obsolete: true
(In reply to arni2033 from comment #5)
> Offtopic
> Actually, if (on Win7) I set HWA -> on, your change causes buttons to have
> height=31px, while with HWA=off their height=30px (as expected). Is that bug
> worth reporting or will it be marked Invalid?

31px according to the DOM or 31px visually? It sounds worth reporting to me, but as I won't be landing the original patch, I expect it'd be useful to have a more minimal testcase for that problem.
(to be clear, that'd be a core bug, and a surprising one at that... though actually, now that I think about it, it might have something to do with the fonts in use and how different methods of rendering those would affect their size, which will cause the "natural" height of the text to be different, which might not in and of itself be a bug)
Comment on attachment 8662341 [details] [diff] [review]
fix text wrapping in buttons on about:support,

(In reply to :Gijs Kruitbosch from comment #7)
> I've changed my mind - this actually breaks a lot of things on OS X, like
> e.g. about:preferences menulists, which have min-height: 20px !important,
> which overrides this and therefore they now end up being 10px smaller.

We can fix that by removing the stray !important, correct? Likewise, other regressions just need to dealt with piece by piece. Not all of that needs to be taken care of by you let alone in this bug; I'm fine with just fixing the most obvious stuff and then landing it to seeing what else breaks.

The hardcoded height in common.css is wrong and we should fix that sooner rather than later, otherwise the crap keeps just piling up as we use common.css in more places.
Attachment #8662341 - Flags: review?(dao) → review-
Attachment #8661744 - Attachment is obsolete: false
Attachment #8662341 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 8662341 [details] [diff] [review]
> fix text wrapping in buttons on about:support,
> 
> (In reply to :Gijs Kruitbosch from comment #7)
> > I've changed my mind - this actually breaks a lot of things on OS X, like
> > e.g. about:preferences menulists, which have min-height: 20px !important,
> > which overrides this and therefore they now end up being 10px smaller.
> 
> We can fix that by removing the stray !important, correct? Likewise, other
> regressions just need to dealt with piece by piece. Not all of that needs to
> be taken care of by you let alone in this bug; I'm fine with just fixing the
> most obvious stuff and then landing it to seeing what else breaks.
> 
> The hardcoded height in common.css is wrong and we should fix that sooner
> rather than later, otherwise the crap keeps just piling up as we use
> common.css in more places.

Alright! I removed the !important (there since hg@1... but I couldn't find anything that broke in browser/, at least), and s/height/min-height in the relevant add-ons stylesheet so that that continues working (see bug 1205620 for fixing the button to actually be invisible, as it's unused, and bug 1205622 for making the test not so ... unreliable).

Let's see how this goes...
(In reply to :Gijs Kruitbosch from comment #9)
> 31px according to the DOM or 31px visually?
According to inspector in devtools (Box Model). And also, I see the buttons twitching when I remove rule "height:30px" (or replace it with rule "min-width:30px"). Filed bug 1205724 with reduced testcase.
https://hg.mozilla.org/mozilla-central/rev/cd8101de67b6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
I have reproduced this bug with Firefox Nightly 43.0a1 (Build ID: 20150915030232) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 44.0a1(Build ID:20151001030236)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Whiteboard: [testday-20151002]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.