Text in button in fullscreen warning box causes overflow

VERIFIED FIXED in Firefox 43

Status

()

defect
--
major
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: arni2033, Assigned: xidorn)

Tracking

(Blocks 1 bug)

Trunk
Firefox 44
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ verified, firefox44 verified)

Details

Attachments

(5 attachments)

Posted file fullScreenBug.html
STR:
1. Download russian localization of Aurora 43
2. Open attached testcase
3. Click "Go fullscreen"

Result:       Text in button in fullscreen warning box causes overflow. And it's even worse on HiDPI
Expectations: There should be no overflow somehow. Maybe there should be 1 small button
              to exit fullscreen (just like in animation inspector)
Flags: needinfo?(quanxunzhen)
// This looks bad even on HiDPI even on EN locale
What's the resolution are you testing?
Flags: needinfo?(arni2033)
I have 1366x768 screen. The attached screenshot was made with dpi = 100%
Flags: needinfo?(arni2033)
[Tracking Requested - why for this release]: annoying UI issue. this warning box is implemented in bug 1160023.
Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box.
Attachment #8667081 - Flags: review?(dao)
Use HTML ones here because the behavior of HTML elements is more predictable and reasonable. It seems to me that several CSS properties I used there do not interact well with XUL elements.
Flags: needinfo?(quanxunzhen)
Assignee: nobody → quanxunzhen
So do you think that 2-line buttons is a good solution? I still can tolerate 1-line buttons with huge width, but 2-line native-styled buttons with huge width look *very* unusual, at least on Windows7.

I haven't seen the CSS so sorry if I mistakenly decided that you made overflowed buttons take 2 lines
(In reply to arni2033 from comment #8)
> So do you think that 2-line buttons is a good solution? I still can tolerate
> 1-line buttons with huge width, but 2-line native-styled buttons with huge
> width look *very* unusual, at least on Windows7.
> 
> I haven't seen the CSS so sorry if I mistakenly decided that you made
> overflowed buttons take 2 lines

That won't usually happen anyway. It happens only when the domain is so long that things cannot be in one line. In that case, both the description and the button would become two lines.

It is doable to make the button always one-line, though. But that could make the whole box thicker, because we would need more lines for the description.
Please the two wrap styles. Verdi, which one do you think is better?
Flags: needinfo?(mverdi)
I meant, please compare the two wrap styles...
Attachment #8667081 - Flags: review?(dao) → review+
  /* We must specify a max-width, otherwise word-wrap:break-word doesn't
     work in descendant <description> and <label> elements. Bug 630864. */
  max-width: 95%;

You removed the comment but not the code. Is this still needed? Bug 630864 only refers to XUL elements.

  /* We must specify a min-width, otherwise word-wrap:break-word doesn't work. Bug 630864. */
  min-width: 1px;

Ditto
(In reply to Dão Gottwald [:dao] from comment #12)
>   /* We must specify a max-width, otherwise word-wrap:break-word doesn't
>      work in descendant <description> and <label> elements. Bug 630864. */
>   max-width: 95%;
> 
> You removed the comment but not the code. Is this still needed? Bug 630864
> only refers to XUL elements.

This code is now used to limit the width of the box, otherwise the box could be too long to fit in the screen. As I need this code no longer because of that bug, I removed the comment but keep the code.

>   /* We must specify a min-width, otherwise word-wrap:break-word doesn't
> work. Bug 630864. */
>   min-width: 1px;
> 
> Ditto

It seems to me this is still relevant. I'm not pretty sure, though. I'll test it again tomorrow to see whether we still need it.
(In reply to Xidorn Quan from comment #7)
> Use HTML ones here because the behavior of HTML elements is more predictable
> and reasonable. It seems to me that several CSS properties I used there do
> not interact well with XUL elements.

XUL has two modes for its text, single-line with optional ellipsis overflow or multiline HTML. (This predates text-overflow: ellipsis; support, which would presumably have made the distinction unnecessary.) Unfortunately the mode is determined by whether the label/description element has a value attribute or not, which is determined by the element's XBL binding. This resulted for example in the creation of bug 880918 to add support for wrapped labels to toolbarbuttons. On the other hand with regular buttons you might be able to add a suitable label as a child of the button instead.

(Not that you support third-party themes any more, but it's possible that including an HTML button with chrome might confuse them.)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> That won't usually happen anyway. It happens only when the domain is so long that things cannot
> be in one line. In that case, both the description and the button would become two lines.
> 
> It is doable to make the button always one-line, though. But that could make the whole box thicker,
> because we would need more lines for the description.
You're saing that just like the (1) button _must_ contain all that text   (2) The fullscreen notification itself _must_ look like table with 2 columns, not even like table with 2 rows

What does mock-up say? If there's no mock-up, I'd suggest at least to add "margin-right:-100%;" to #fullscreen-warning. That would make that panel really flexible and eliminate those 3-line button cases
>>>   http://ssmaker.ru/bc0b99ac.png   <<<
>also http://ssmaker.ru/a38bd73c.png (HiDPI) and http://ssmaker.ru/658b8c69.png (russian DevEdition)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> Created attachment 8667247 [details]
> comparison of button wrap style
> 
> Please the two wrap styles. Verdi, which one do you think is better?

Is it possible to stack the url over the button so each one has more room and less need for wrapping?
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #16)
> to stack the url over the button so each one has more room and less need for wrapping
You're suggesting to always have at least 2 lines in notification while in many cases it's possible to avoid wrapping at all. Currently fullscreen notification can't take more than half of screen width due to bad styling, but it was designed to take up to 95% of screen width.
I described how it could be fixed in my comment 15 (please don't ignore it as well as "#fullscreen-warning { max-width:95%; }" in browser stylesheet)
(In reply to arni2033 from comment #15)
> What does mock-up say? If there's no mock-up, I'd suggest at least to add
> "margin-right:-100%;" to #fullscreen-warning. That would make that panel
> really flexible and eliminate those 3-line button cases
> >>>   http://ssmaker.ru/bc0b99ac.png   <<<
> >also http://ssmaker.ru/a38bd73c.png (HiDPI) and http://ssmaker.ru/658b8c69.png (russian DevEdition)

My patch works exactly the same as the last picture you provide for your resolution (1366x768) and the given (not ridiculous long) domain name.

The attachment I uploaded was for the extreme case which displays a unreasonably long domain in 800x600 resolution (which is the lowest resolution our users may have according to the telemetry).
(In reply to neil@parkwaycc.co.uk from comment #14)
> (In reply to Xidorn Quan from comment #7)
> > Use HTML ones here because the behavior of HTML elements is more predictable
> > and reasonable. It seems to me that several CSS properties I used there do
> > not interact well with XUL elements.
> 
> (Not that you support third-party themes any more, but it's possible that
> including an HTML button with chrome might confuse them.)

So your suggestion is that, using HTML elements is otherwise fine, but we should probably still use the XUL button in any case, right?

> XUL has two modes for its text, single-line with optional ellipsis overflow
> or multiline HTML. (This predates text-overflow: ellipsis; support, which
> would presumably have made the distinction unnecessary.) Unfortunately the
> mode is determined by whether the label/description element has a value
> attribute or not, which is determined by the element's XBL binding. This
> resulted for example in the creation of bug 880918 to add support for
> wrapped labels to toolbarbuttons. On the other hand with regular buttons you
> might be able to add a suitable label as a child of the button instead.

It seems just moving the text from the value attribute to its content text makes it wrap properly.
Flags: needinfo?(neil)
(In reply to Verdi [:verdi] from comment #16)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> > Created attachment 8667247 [details]
> > comparison of button wrap style
> > 
> > Please the two wrap styles. Verdi, which one do you think is better?
> 
> Is it possible to stack the url over the button so each one has more room
> and less need for wrapping?

Could you provide a mockup for that?

Also note that, I don't really think this could happen frequently after I actually expand the max width to 95% of the screen width. As I mentioned in comment 18, a common screen width of 1366px is enough for a reasonably long domain name like bug1209060.bmoattachments.org. Thus the wrapping is just for having a sensible enough behavior for rare cases.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> >   /* We must specify a min-width, otherwise word-wrap:break-word doesn't
> > work. Bug 630864. */
> >   min-width: 1px;
> > 
> > Ditto
> 
> It seems to me this is still relevant. I'm not pretty sure, though. I'll
> test it again tomorrow to see whether we still need it.

Yes, it is still needed because of the reason in the comment. As the comment says, if I remove the min-width here, word-wrap: break-word would stop working.
(In reply to Xidorn Quan from comment #19)
> (In reply to comment #14)
> > (Not that you support third-party themes any more, but it's possible that
> > including an HTML button with chrome might confuse them.)
> 
> So your suggestion is that, using HTML elements is otherwise fine, but we
> should probably still use the XUL button in any case, right?

I'm no browser peer but it would be my personal preference.

> > the mode is determined by whether the label/description element has a value
> > attribute or not, which is determined by the element's XBL binding. This
> > resulted for example in the creation of bug 880918 to add support for
> > wrapped labels to toolbarbuttons. On the other hand with regular buttons you
> > might be able to add a suitable label as a child of the button instead.
> 
> It seems just moving the text from the value attribute to its content text
> makes it wrap properly.

Indeed. (Of course that assumes you have direct control of the label element, which isn't true for the automatically generated element, thus the necessity of providing your own label element explicitly, which is fortunately permitted in the case of a button.)
Flags: needinfo?(neil)
Blocks: 1209831
Comment on attachment 8667081 [details]
MozReview Request: Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box.

Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box.
Attachment #8667081 - Flags: review+ → review?(dao)
Comment on attachment 8667081 [details]
MozReview Request: Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box.

Switching everything to html except for the button doesn't seem like such a good idea to me. I'd prefer to have it all in either html or xul.
Attachment #8667081 - Flags: review?(dao)
Component: Widget → General
Product: Core → Firefox
(In reply to Dão Gottwald [:dao] from comment #24)
> Switching everything to html except for the button doesn't seem like such a
> good idea to me. I'd prefer to have it all in either html or xul.

Ah, okay, then I'll just land the previous patch.
https://hg.mozilla.org/integration/fx-team/rev/fb34a505c11eb7eaba36002ca6a59c29bc127aba
Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box. r=dao
(In reply to Xidorn Quan from comment #19)
> So your suggestion is that, using HTML elements is otherwise fine, but we
> should probably still use the XUL button in any case, right?

Sorry, I misread that. If adding a <label>wrapped text here</label> child to the XUL button worked, then I would have gone with that, but I don't know why it was necessary to change the other label/description to HTML either.
(In reply to neil@parkwaycc.co.uk from comment #27)
> (In reply to Xidorn Quan from comment #19)
> > So your suggestion is that, using HTML elements is otherwise fine, but we
> > should probably still use the XUL button in any case, right?
> 
> Sorry, I misread that. If adding a <label>wrapped text here</label> child to
> the XUL button worked, then I would have gone with that, but I don't know
> why it was necessary to change the other label/description to HTML either.

The issue is not only on the button. It seems "word-wrap: break-word" doesn't work on <label> as well. When the domain name is long enough, it would just let the text overflow instead of making it break.

So I still think the behavior of HTML elements is more predictable (and better documented in specs) especially when interacting with CSS properties.

(In addition, actually it seems while using text inside XUL button works, adding an additional <label> inside stops working. The label makes the text overflow whatever property I apply on it.)
https://hg.mozilla.org/mozilla-central/rev/fb34a505c11e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Posted patch patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 1160023
[User impact if declined]: see broken fullscreen notification UI with long domain name or in certain locale
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: not sure how risky is it to use html element in chrome UI. I guess not too risky, though
[String/UUID change made/needed]: n/a
Attachment #8668884 - Flags: approval-mozilla-aurora?
Comment on attachment 8668884 [details] [diff] [review]
patch

Approved for uplift to aurora.
Attachment #8668884 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Let's verify this fix and make sure nothing odd happens on aurora 43... :)
Flags: qe-verify+
Depends on: 1216445
No longer depends on: 1216445
Verified as fixed on:
* latest Nightly - build ID 20151022030546
* latest Aurora - build ID 20151022004116

Testing was performed with the Russian locale on Windows 7 x64, Windows 10 x64, Ubuntu 12.04 x86 and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1205117
Has STR: --- → yes
You need to log in before you can comment on or make changes to this bug.