1209060 - Text in button in fullscreen warning box causes overflow

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[arni2033]

Attachment: 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)

Comment 1

[arni2033]

Attachment: screenshot - Text in button in fullscreen warning box causes overflow.png

Comment 2

[arni2033]

// This looks bad even on HiDPI even on EN locale

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

What's the resolution are you testing?

Comment 4

[arni2033]

I have 1366x768 screen. The attached screenshot was made with dpi = 100%

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

[Tracking Requested - why for this release]: annoying UI issue. this warning box is implemented in bug 1160023.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: 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.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 8

[arni2033]

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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: comparison of button wrap style

Please the two wrap styles. Verdi, which one do you think is better?

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

I meant, please compare the two wrap styles...

Comment 12

[Dão Gottwald [::dao]]

  /* 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

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 14

[[e-mail]]

(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.)

Comment 15

[arni2033]

(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)

Comment 16

[Verdi [:verdi] Best to slack me]

(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?

Comment 17

[arni2033]

(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)

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(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).

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

(In reply to [e-mail] 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.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 22

[[e-mail]]

(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.)

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 24

[Dão Gottwald [::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.

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/fx-team/rev/fb34a505c11eb7eaba36002ca6a59c29bc127aba
Bug 1209060 - Use html elements instead of xul ones for fullscreen warning box. r=dao

Comment 27

[[e-mail]]

(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.

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

(In reply to [e-mail] 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.)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb34a505c11e

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

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

Comment 31

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8668884 [details] [diff] [review]
patch

Approved for uplift to aurora.

Comment 32

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Let's verify this fix and make sure nothing odd happens on aurora 43... :)

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9dfec714d1df

Comment 34

[Cornel Ionce [:noni] [Hubs QA]]

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.