Closed Bug 1010675 Opened 10 years ago Closed 10 years ago

<button> content overflows into padding when overflow:hidden, text-overflow: ellipsis and white-space: nowrap is specified

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla39
blocking-b2g 2.2+
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: chen.cong, Assigned: dbaron)

References

Details

(Whiteboard: [priority] interaction-design)

Attachments

(15 files, 2 obsolete files)

(deleted), image/png
Details
(deleted), text/x-github-pull-request
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), text/x-github-pull-request
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), text/html
Details
(deleted), text/html; charset=UTF-8
Details
(deleted), text/html; charset=UTF-8
Details
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.137 Safari/537.36 Steps to reproduce: 1) Using one long filename MP3 as the ringtone; 2) Enter settings to see the ringtone display; Actual results: There is overlapping in the UI. Expected results: No overlapping in the UI. The long filename should be cut down as "XXXX..."
OS: All → Gonk (Firefox OS)
Priority: -- → P1
Hardware: All → ARM
Attached image overlapping UI.PNG (deleted) —
ni Howie Hi Howie, this looks like a general setting display issue, could you help to find someone to check? Thanks
Flags: needinfo?(hochang)
Hi Tim, I put this as priority bug and please assign mentor to this, thanks.
blocking-b2g: --- → backlog
Flags: needinfo?(hochang) → needinfo?(timdream)
Whiteboard: [priority]
Thanks Howie and Tim, this issue is reported by carrier while doing the IOT test for Open C. Could be a minor UI defect but since it is found during the IOT, would be much appreciated if you can check it and provide some feedbacks Thanks
What's the desired UX? Arthur, is this a good first bug?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(timdream)
Flags: needinfo?(firefoxos-ux-bugzilla)
Flags: needinfo?(arthur.chen)
Sorry, Arthur is PTO ... I should ask EJ instead.
Flags: needinfo?(arthur.chen) → needinfo?(ejchen)
I checked the behavior between master & v1.3 and noticed that master works well if the filename is too long. It may be `overflow` problem in CSS and I think we can fix it in v1.3. Note: I would take the bug first and please retake it if you are interested in this bug, if not, I would fix it later :)
Assignee: nobody → ejchen
Flags: needinfo?(ejchen)
Whiteboard: [priority] → [priority][mentor=ejchen][mentor-lang=zh]
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #7) > I checked the behavior between master & v1.3 and noticed that master works > well if the filename is too long. > > It may be `overflow` problem in CSS and I think we can fix it in v1.3. > > Note: > > I would take the bug first and please retake it if you are interested in > this bug, if not, I would fix it later :) Hi EJ, this looks fun, could I try to take it with your help? looks like i should start with checking the css icon-view class?
Flags: needinfo?(ejchen)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #8) > Hi EJ, this looks fun, could I try to take it with your help? > > looks like i should start with checking the css icon-view class? Hi Vance, I have some information for you ! The reason why there is no bug like this in master is because Settings App is using updated version of buttons.css. (style_unstable) And after comparing `style_unstable/buttons.css` with `style/buttons.css`, you can notice the root cause of this bug would be https://github.com/mozilla-b2g/gaia/blob/master/shared/style/buttons.css#L124. @Arnau, because you are the main contributor in BB and you know more about the histories, can you provide some information here about this bug :) ? In order to minimize the influence scale, I think we can override this (overflow & white-space) in `settings.css`, what do you think ?
Flags: needinfo?(ejchen) → needinfo?(arnau)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #8) > Hi EJ, this looks fun, could I try to take it with your help? > > looks like i should start with checking the css icon-view class? BTW, Vance, you can try to make a temp patch first based on my comment #9 to make sure the bug got fixed :)
Flagging Tif to describe the desired UX.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(tshakespeare)
Truncation rules need to apply here and we should show an ellipsis when we are about out of room. Let me know if you need anything else and please flag me for UI-review when there is a patch to take a look at. Thanks!
Flags: needinfo?(tshakespeare)
Whiteboard: [priority][mentor=ejchen][mentor-lang=zh] → [priority][mentor=ejchen][mentor-lang=zh] interaction-design
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #10) > (In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #8) > > Hi EJ, this looks fun, could I try to take it with your help? > > > > looks like i should start with checking the css icon-view class? > > BTW, > > Vance, you can try to make a temp patch first based on my comment #9 to make > sure the bug got fixed :) OK doing that now, thanks for the tips!
Attached file patch (deleted) —
Attachment #8426128 - Flags: review?(ejchen)
Hi EJ - Probably this one won't land in 1.3 but still i made a patch, if you have time, could you help to review and comment? Thanks Vance
Flags: needinfo?(ejchen)
Sorry guys for not answering before. I gave it a try in gaia master, but the buttons have changed and I could not reproduce this 100%. The proposed overflow patch, does the trick for 1.3 But we should still modify the new buttons (in gaia master) as the text goes under the arrow icon.
Flags: needinfo?(arnau)
Comment on attachment 8426128 [details] patch Great start Vance! I looked at your patch on my Flame and the issue of wrapping text is gone - it stays within the button. Awesome! Looks like we're missing the ellipsis that we need when truncating text. And to be really nitty, we could probably fit a few more characters in there before the ellipsis? Just a suggestion :) Thanks Vance for working on this - it was bugging me! My info just in case: Gaia 5bea67f2db3e368446048d2f6dfde42622413f4b Gecko https://hg.mozilla.org/mozilla-central/rev/a6f36c31aa8f BuildID 20140521040203 Version 32.0a1 ro.build.version.incremental=76 ro.build.date=Mon Apr 14 14:02:50 CST 2014
Attachment #8426128 - Flags: ui-review-
Hi Vance, I left some comments on Github and plz check it :)
Flags: needinfo?(ejchen)
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #18) > Hi Vance, > > I left some comments on Github and plz check it :) Thanks EJ, already modified my patch based on your comments, please help to check again~ Thanks
hi vance, This patch will cause the "settings"->"Media storage" -> Default media location display "...", it may be display "Internal" or "SD Card". just like the picture "wrong display storage"
Flags: needinfo?(vchen)
Attached image wrong display storage.png (deleted) —
(In reply to chencong from comment #20) > hi vance, > > This patch will cause the "settings"->"Media storage" -> Default media > location display "...", it may be display "Internal" or "SD Card". just > like the picture "wrong display storage" Hi Chen Cong - there are two version of the patch, which version are you using? Thanks Vance
Flags: needinfo?(vchen)
Flags: needinfo?(chen.cong)
I used the "apps/settings/style/settings.css" file modification. "Bug 1010675 - Override text-overflow property in settings.css for rin… … 9fa98ae" . Is this true?
Flags: needinfo?(chen.cong)
(In reply to chencong from comment #23) > I used the "apps/settings/style/settings.css" file modification. "Bug > 1010675 - Override text-overflow property in settings.css for rin… … > 9fa98ae" . Is this true? In that case, please try 5bea67f: https://github.com/vanceyen/gaia/commit/5bea67f2db3e368446048d2f6dfde42622413f4b Thanks Vance
Flags: needinfo?(chen.cong)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #24) > In that case, please try 5bea67f: > > https://github.com/vanceyen/gaia/commit/ > 5bea67f2db3e368446048d2f6dfde42622413f4b > > Thanks > > Vance Hi Vance, after checking BB CSS, it seems that we would wrap `select` inside `span` to make this problem happen. Because it is v1.3 patch, after considerations, I think it is still better not to change too much now. Let's just use your previous rule with my suggested CSS properties. Thanks !!
Hi vance, I used another patch. It is OK. Thanks!
Flags: needinfo?(chen.cong)
Comment on attachment 8426128 [details] patch I would remove the r tag first and don't hesitate to ping me again when you update your patch. Big thanks, Vance :)
Attachment #8426128 - Flags: review?(ejchen)
Attached image Ringtone Name (deleted) —
Hello again! I took a look at the patch up above again and took a screenshot of what I see. - the padding on the left and right side of the name needs to be adjusted. On the left the "B" should be in line with the "[" of the Alerts item. On the right, the "..." should occur before the arrow. - the alerts sound appears to be broken now. Instead of the name of the sound, it shows "[object] [Object]" I've NI'd Carol, the visual designer of Settings, to assist more with padding issues.
Flags: needinfo?(chuang)
(In reply to Tiffanie Shakespeare from comment #28) > - the alerts sound appears to be broken now. Instead of the name of the > sound, it shows "[object] [Object]" Hi Tiffanie, there is only CSS change on this patch, are you sure the patch would cause this problem ? or it may be something wrong there ?
(In reply to Tiffanie Shakespeare from comment #28) > - the padding on the left and right side of the name needs to be adjusted. > On the left the "B" should be in line with the "[" of the Alerts item. On > the right, the "..." should occur before the arrow. This problem exists on all buttons in Gaia and I think we have to open another bug for it (Building Block). For this bug, it mainly focuses on fixing the overlapping UI problem, I think it would be better to split these two bugs. What do you think, Tiffanie :) ?
Forgot to put ni on Tiffanie :)
Flags: needinfo?(tshakespeare)
Sorry - I'm not a developer so I can't answer that. But I do know that when I install master or any other patch, I see the name of the alert, but with this patch I see the screenshot I posted. (In reply to EJ Chen [:eragonj][:小龍哥] from comment #29) > (In reply to Tiffanie Shakespeare from comment #28) > > - the alerts sound appears to be broken now. Instead of the name of the > > sound, it shows "[object] [Object]" > > Hi Tiffanie, there is only CSS change on this patch, are you sure the patch > would cause this problem ? or it may be something wrong there ?
Flags: needinfo?(tshakespeare)
Attached image Bluetooth settings (deleted) —
Which problem? The padding on the left or the right? Looking around in settings, all the other buttons have the same left padding as the alert (an example is attached) except for the ringer which is too far to the left. For right, I wasn't able to find any other example of truncation occurring. Also, I thought this fix would apply to the button building block and not just this specific instance. I could see a case for applying truncation rules to all buttons in case they run into them. NI'ing Casey who is working on Building Blocks. Thanks EJ! (In reply to EJ Chen [:eragonj][:小龍哥] from comment #30) > (In reply to Tiffanie Shakespeare from comment #28) > > - the padding on the left and right side of the name needs to be adjusted. > > On the left the "B" should be in line with the "[" of the Alerts item. On > > the right, the "..." should occur before the arrow. > > This problem exists on all buttons in Gaia and I think we have to open > another bug for it (Building Block). > > For this bug, it mainly focuses on fixing the overlapping UI problem, I > think it would be better to split these two bugs. > > What do you think, Tiffanie :) ?
Flags: needinfo?(kyee)
(In reply to Tiffanie Shakespeare from comment #33) > Created attachment 8430429 [details] > Bluetooth settings > > Which problem? The padding on the left or the right? > > Looking around in settings, all the other buttons have the same left padding > as the alert (an example is attached) except for the ringer which is too far > to the left. > > For right, I wasn't able to find any other example of truncation occurring. > > Also, I thought this fix would apply to the button building block and not > just this specific instance. I could see a case for applying truncation > rules to all buttons in case they run into them. > > NI'ing Casey who is working on Building Blocks. > > Thanks EJ! Bad news Tiffanie, Please try this link first with Google Chrome, Safari and Firefox : http://jsbin.com/juketiqa/1 You can notice that our Gecko would handle the padding differently especially in *button* element to make the left / right padding missing. If you also see this syndrome there on your browser, I would try to make a bug for gecko and this bug will only focuses on making the text overflow without fixing padding issue. Hope this make sense to you :)
Flags: needinfo?(tshakespeare)
Flagging Harly instead of Casey. Casey is not working on BB.
Flags: needinfo?(kyee) → needinfo?(hhsu)
Also flagging Pavel to see if he can help on the CSS issue at all.
Flags: needinfo?(pivanov)
After talking to Harley, as is to late to fix this bug for 1.3 I will create a patch to fix it for the new buttons (2.0)
Flags: needinfo?(tshakespeare)
Flags: needinfo?(pivanov)
Flags: needinfo?(hhsu)
Flags: needinfo?(chuang)
(In reply to Arnau March [:arnau] from comment #37) > After talking to Harley, as is to late to fix this bug for 1.3 I will create > a patch to fix it for the new buttons (2.0) So, Arnau, should we make this bug as invalid ?
We should maybe rename the bug and attach a new capture.
(In reply to Arnau March [:arnau] from comment #39) > We should maybe rename the bug and attach a new capture. Just updated the title. So Arnau, are you going to take this bug ?
Summary: [OPEN C_1.3][settings] When using MP3 Ringtone with long filename of MP3 - there is overlapping in the UI → [BB][settings] Truncate text in buttons and keep paddings on it
Assignee: ejchen → pacorampas
I have assigned it to Paco, as this one is a BB issue, I can take the review.
(In reply to Arnau March [:arnau] from comment #41) > I have assigned it to Paco, as this one is a BB issue, I can take the review. OK, thanks for your help, Arnau :] +++
Attached image word-wrap-btn-settings.png (deleted) —
Attached file patch in github (deleted) —
Attachment #8434073 - Flags: review?(arnau)
(In reply to Paco Rampas [:paco] from comment #44) > Created attachment 8434073 [details] > patch in github Paco, I still can't get it why padding-left will get lost when text is overflow with ellipsis appended. Do you know why ? I put a link on comment 34 to show this syndrome.
Flags: needinfo?(pacorampas)
> I still can't get it why padding-left will get lost when text is overflow > with ellipsis appended. Do you know why ? Hello, could you get the patch now? I think the issue is fixed. https://github.com/mozilla-b2g/gaia/pull/20002
Flags: needinfo?(pacorampas)
NI'ing EJ per Paco's comment #46. It would be awesome if we could fix this for the 2.0 release.
Flags: needinfo?(ejchen)
We branch for 2.0 today, allegedly, so if this doesn't make it 2.1 will be fine. Also make sure someone from L10N knows this is coming (if they don't already).
I think :paco misunderstood my meaning. The patch is ok for me and I just want to know what happened based on my comment 34.
Flags: needinfo?(ejchen)
BTW :paco, because currently we use `background-image` to show the right arrow inside the button, when text is truncated, our dots (... <- like this) will overlap with the arrow. I am thinking whether it is ok to use `text-overflow: "... "` (<- with two more spaces) to fix this for this case ? I can't think a better way to fix this, if you have any better idea, please share here. Thanks :P
Flags: needinfo?(pacorampas)
:eragon, thanks for the trick: `text-overflow: "... "` We will try that :)
Flags: needinfo?(pacorampas)
(In reply to Arnau March [:arnau] from comment #51) > :eragon, > thanks for the trick: `text-overflow: "... "` > We will try that :) Thanks Arnau ! By the way, that really works on the device (I tested with App manager on my flame). Hope this helps :P
Will need to test with rtl, not sure if it will be something like: "... Text"
Attached image rtl.png (deleted) —
Yes it does work ! We only have to make sure the number of spaces is ok (5 ~ 6 spaces in this screenshot), because the word size is a little bit too thin.
Awesome! we will update the patch today :)
> `text-overflow: "... "` (<- with two more spaces) to fix this for this case > ? It is great!! I have updated the patch with this solution.
A few things on intl here: Three dots isn't the right ellipsis, basically anywhere. There's a unicode glyph for &hellip;. 2026. I also recall that there's languages for which there's a mid-level ellipse, but I can't find an example of that in http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=intl.ellipsis. Jonathan, Simon? For RTL, we should really check BIDI, i.e. a variety of mixed directionality in the name of the tone, in both LTR and RTL interfaces. Not sure if the spacing of ' ' will depend on the selected font in the UI and thus depend on language, or if that's actually always the same thing.
(In reply to Axel Hecht [:Pike] from comment #57) > A few things on intl here: > > Three dots isn't the right ellipsis, basically anywhere. There's a unicode > glyph for &hellip;. 2026. > Yes for sure, that would be better to use specific character for ellipsis. > I also recall that there's languages for which there's a mid-level ellipse, > but I can't find an example of that in > http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=intl.ellipsis. > Jonathan, Simon? > > For RTL, we should really check BIDI, i.e. a variety of mixed directionality > in the name of the tone, in both LTR and RTL interfaces. > Is there any demonstration for this case ? I am not familiar with this and want to learn :P > Not sure if the spacing of ' ' will depend on the selected font in the UI > and thus depend on language, or if that's actually always the same thing. This is part of my concerns too, so this is one of temporary proposal coming from my mind. Do you have any better ideas / suggestions for this CSS problems ?
(In reply to Axel Hecht [:Pike] from comment #57) > I also recall that there's languages for which there's a mid-level ellipse, > but I can't find an example of that in > http://mxr.mozilla.org/l10n-mozilla-aurora/search?string=intl.ellipsis. > Jonathan, Simon? Japanese? I think Chinese uses 6 dots vertically centered, but not sure how that applies on computers. In zh-CN I can only find 2 strings with a double ellipsis (not midline) http://transvision.mozfr.org/?recherche=%E2%80%A6%E2%80%A6&repo=central&sourcelocale=en-US&locale=zh-CN&search_type=strings
(In reply to Francesco Lodolo [:flod] from comment #59) > Japanese? http://en.wikipedia.org/wiki/Japanese_punctuation#Ellipsis But Microsoft in its style guide requires to use the same ellipses used in English.
We should be able to use -moz-padding-end instead of padding-right to get the padding in the correct position for RTL. I'm not sure what the options are for localizing the ellipsis character here -- can we just use the existing intl.ellipsis?
That would require us to actually include gecko localizations, which we're not doing right now. We could put the ellipsis in the localized files and use CSS DOM to insert/update the CSS rule that has the content, I guess.
Attached image buttons.png (deleted) —
David, (Please see the attached image) I've noticed some inconsistencies while trying to fix this bug, as buttons behaves different than other tags. The captures are from buttons Building block in: gaia/shared/style/buttons/index.html I've tried: button::-moz-focus-inner { padding: 0 1.2rem; } button { padding: 0; } instead of: button { padding: 0 1.2rem; } And it works, but in some apps where they are already defining a padding for the button, ::-moz-focus-inner is adding some extra 1.2rem padding. Do you know how could we solve that?
Could you upload the testcase you used to generate that image? Because if our behavior is incompatible with other browsers (which seems likely), we could change our behavior, and it would probably be a relatively simple fix in nsGfxButtonControlFrame (and/or nsHTMLButtonControlFrame), but if it matches other browsers we'd likely need to stick to it.
Flags: needinfo?(dbaron)
Attached file test.html (deleted) —
David, in this simple example you could see the difference between Firefox (Nightly 33.0a1) and Chrome (37.0.2043.0 canary). When the button has it's default style (attachment 8439217 [details]) padding is not affecting, but it does when removing the border (attachment 8439218 [details]) In Chrome padding is always applied, also when text is truncated. IMHO that should be the right behavior.
Attached file some more examples (obsolete) (deleted) —
Based on Chrome's behavior, it certainly looks like we could eliminate the extrawidth/extraleft/extraright business in http://hg.mozilla.org/mozilla-central/file/c482c28b35b6/layout/forms/nsHTMLButtonControlFrame.cpp#l237 I'm curious whether that matches IE and Safari as well, though. I'm also curious how urgent this is. I don't have time this week to fix this; perhaps jwatt would be able to look sooner than I can.
Flags: needinfo?(dbaron) → needinfo?(jwatt)
No worries, is not that urgent. Safari is showing exactly the same as Chrome. No ie installed :( Thanks a lot!
(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-4) from comment #68) > Created attachment 8439261 [details] > some more examples > > Based on Chrome's behavior, it certainly looks like we could eliminate the > extrawidth/extraleft/extraright business in > http://hg.mozilla.org/mozilla-central/file/c482c28b35b6/layout/forms/ > nsHTMLButtonControlFrame.cpp#l237 David, I mentioned this problem on Comment 34, and you can also give it a try !
:eragonj, Sorry I did not read your comment before, but good we got to the same conclusion ;)
Comment on attachment 8434073 [details] patch in github Clearing review for this patch
Attachment #8434073 - Flags: review?(arnau)
Mentor: ejchen
Whiteboard: [priority][mentor=ejchen][mentor-lang=zh] interaction-design → [priority][mentor-lang=zh] interaction-design
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #68) > I'm also curious how urgent this is. I don't have time this week to fix > this; perhaps jwatt would be able to look sooner than I can. Unfortunately I've not managed that, and it's going to be at least another four weeks before I could spend time on this (vacation time in there), so clearing needinfo for now.
Flags: needinfo?(jwatt)
Assignee: pacorampas → nobody
Blocks: 1036131
David, could you please look into that again? We have found more bugs in gaia depending on that issue. Thanks!
Flags: needinfo?(dbaron)
noming to 2.0 as it blocks a blocker
blocking-b2g: backlog → 2.0?
blocking-b2g: 2.0? → 2.0+
Hi Paco, do you mind taking this again? Somehow the assignee is removed. Thank you!
Flags: needinfo?(pacorampas)
David Baron or Jonathan Watt should take care on this as is a platform issue.
Flags: needinfo?(pacorampas)
Right, it's on platform now, sorry I missed the info. Hi David, unfortunately this becomes a bit urgent now since it blocks a blocker, would you have time this? If no, anyone else you recommend besides jwatt? Many thanks!
Flags: needinfo?(dbaron)
I don't understand why this bug should block bug 1036131. If Jose is talking about the platform fix is blocking that bug, we should change the summary of this bug and the product/component to DOM::Layout. It's been decided on comment 3 this bug is not a blocker in the view of Settings app. Jose, please fix your dependency by filing a new bug to track the platform issue. Thanks.
blocking-b2g: 2.0+ → backlog
Flags: needinfo?(jmcf)
Ni bbajaj so your side of people can improve the quality of triage decision by reading the bug thoroughly.
Flags: needinfo?(bbajaj)
Component: Gaia::Settings → Graphics
Flags: needinfo?(jmcf)
OS: Gonk (Firefox OS) → All
Product: Firefox OS → Core
blocking-b2g: backlog → 2.0?
Summary: [BB][settings] Truncate text in buttons and keep paddings on it → Inconsistent behavior of paddings in button element
I have changed the title of the bug and the Product and Component.
Mentor: ejchen
Component: Graphics → Layout
Whiteboard: [priority][mentor-lang=zh] interaction-design → [priority] interaction-design
> I'm curious whether that matches IE and Safari as well, though. IE seems to apply the padding as well. If people want to change this behavior, they need to look carefully at small-button testcases like attachment 106247 [details] (albeit without native theming, perhaps). Also, it's worth separately considering behavior with/without native theming, since in WebKit-based browsers it can differ radically...
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #81) > Ni bbajaj so your side of people can improve the quality of triage decision > by reading the bug thoroughly. https://bugzilla.mozilla.org/show_bug.cgi?id=1010675#c76 was considered when this was triaged which comment #79 also took into account. And looks like the details changed later here after further questioning. We try to make the best decision given the information at hand and when more information comes to light that may change as you see. And it still looks like this is needed by dependency. :jose can you respond to comment #80 and help understand if the dependency that is set here still applies ?
Flags: needinfo?(bbajaj) → needinfo?(jmcf)
(In reply to howie [:howie] from comment #79) > Right, it's on platform now, sorry I missed the info. Hi David, > unfortunately this becomes a bit urgent now since it blocks a blocker, would > you have time this? If no, anyone else you recommend besides jwatt? Many > thanks! I'm a bit skeptical of this being a blocker; this is asking for a behavior change to how form controls respond to CSS styling, which is an area for which there is no specification, and where browsers differ a good bit. Changes to that sort of thing are risky and not the sort of thing I'd want to land on aurora a week before it merges to beta. Doing that has more Web-compatibility risk than landing such changes sooner. Yes, the Gecko button behavior here isn't great and could probably be improved, but we need to be somewhat careful doing it, at least in terms of doing thorough testing of other browsers to understand the space in which we're constrained to change it. I'll investigate it a bit more to see if there's something that seems like it will clearly be a safe change, though. Is there something you could do to work around this on the Gaia side instead?
Flags: needinfo?(dbaron)
Attached file some more examples (deleted) —
Attachment #8439261 - Attachment is obsolete: true
Attached patch patch to not erode the padding of the button (obsolete) (deleted) — Splinter Review
This seems (unless I'm missing something) to make us uniformly more consistent with other browsers (Chrome, IE) in the cases I'm looking at; I'm inclined to think this is the right thing to do primarily on that basis, and because it's a simpler and more understandable behavior. What other cases do you think need to be considered? Are you ok with this change?
Attachment #8456571 - Flags: review?(bzbarsky)
Flags: needinfo?(jmcf)
Comment on attachment 8456571 [details] [diff] [review] patch to not erode the padding of the button The really big worry here is small buttons and whether text in them is readable, but it looks like that now sucks in all browsers, so at least we'd be no worse than others. r=me Do we want to allow eroding the focuspadding, thought?
Attachment #8456571 - Flags: review?(bzbarsky) → review+
That could get solved by removing the text-overflow and white-space properties, so we are not allowing text truncation and instead we allow multiple lines in a button. But I'll need UX approval as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=1036147#c10
blocking 2.0-. gecko risk is high, and this situation really doesnt block functionality being this late in the release.
blocking-b2g: 2.0? → -
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #85) > (In reply to howie [:howie] from comment #79) > > Right, it's on platform now, sorry I missed the info. Hi David, > > unfortunately this becomes a bit urgent now since it blocks a blocker, would > > you have time this? If no, anyone else you recommend besides jwatt? Many > > thanks! > > I'm a bit skeptical of this being a blocker; this is asking for a behavior > change to how form controls respond to CSS styling, which is an area for > which there is no specification, and where browsers differ a good bit. > Changes to that sort of thing are risky and not the sort of thing I'd want > to land on aurora a week before it merges to beta. Doing that has more > Web-compatibility risk than landing such changes sooner. > > Yes, the Gecko button behavior here isn't great and could probably be > improved, but we need to be somewhat careful doing it, at least in terms of > doing thorough testing of other browsers to understand the space in which > we're constrained to change it. I'll investigate it a bit more to see if > there's something that seems like it will clearly be a safe change, though. > > Is there something you could do to work around this on the Gaia side instead? David, we have found a work around for gaia buttons in bug 1036147. I can understand a patch in gecko could have a big impact, but would be great if you could fix that whenever works best for you :) Thanks.
Flags: needinfo?(dbaron)
(In reply to Boris Zbarsky [:bz] from comment #89) > Do we want to allow eroding the focuspadding, thought? Curently we don't, and I don't think we want to. Are you suggesting we do want to?
Flags: needinfo?(dbaron) → needinfo?(bzbarsky)
I think we might, if we don't allow eroding the normal padding. For small buttons the focuspadding takes up an appreciable part of the width, and since other UAs have no such thing we can easily get into situations where the text is fully visible in other UAs but not in Gecko.
Flags: needinfo?(bzbarsky)
I guess it's not clear to me how we end up with sensible behavior if we go down that path; it seems like we get the problems in this bug (with inconsistent alignment, etc.) back again.
Only by 1-2px, right? And note that consistent alignment with other browsers would mean 0 focuspadding. Something we should do anyway....
Copying the way better summary from bug 1053448
Summary: Inconsistent behavior of paddings in button element → <button> content overflows into padding when overflow:hidden, text-overflow: ellipsis and white-space: nowrap is specified
Current version of the patch is: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/2fbce19c28d9/button-dont-erode-padding (I still need to figure out what to do about the focusPadding.)
Blocks: 1140645
put 2.2+ as this blocks a blocker.
Blocks: 1135161
blocking-b2g: - → 2.2+
With tests, but also with a FIXME for a failure of an existing test that might be a problem: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ef40ad370ab1/button-dont-erode-padding
Hi David, I'd put you as assignee if you don't mind, thanks!
Assignee: nobody → dbaron
(In reply to David Baron [:dbaron] (UTC-7) from comment #104) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=664b1d6e6625 I've fixed the failures and tested that the fixes work locally. (Also, the reftest-2 job did finish, and BuildAPI says it's green; it just didn't get to treeherder.)
Note that this replaces the code that allows eroding the space with new code that reduces the focusPadding value. (Also, we previously didn't count the focusPadding towards what could be eroded, which meant we wouldn't quite get to the edge of the padding and border, because we weren't counting the extra for the focusPadding.) The existing reftests that I'm changing from == to != are ones that were specifically testing issues related to erosion of padding. The change to 491180-{1,2}-ref.html is because we now *do* erode the focusPadding, which is 3px in the horizontal dimensions (see the button::-moz-focus-inner styles in forms.css), and that was the only nonzero style on the button in 491180-{1,2}.html.
Attachment #8578758 - Flags: review?(dholbert)
Attachment #8456571 - Attachment is obsolete: true
Hardware: ARM → All
Version: unspecified → Trunk
Comment on attachment 8578758 [details] [diff] [review] Stop allowing button contents to overflow into the CSS padding and border (although still allow them to overflow into our internal focuspadding when the min-content width says the contents don't fit) Looks good. Thanks for the reftest-tweak explanations, too.
Attachment #8578758 - Flags: review?(dholbert) → review+
And a perhaps-intermittent failure of 359903-1.html on 10.10 debug, https://treeherder.mozilla.org/logviewer.html#?job_id=7826496&repo=mozilla-inbound
(In reply to Phil Ringnalda (:philor) from comment #110) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/2eca228bc999, your > reftests failed in the outer corners on Android, > http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/ > reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/mobile/ > tinderbox-builds/mozilla-inbound-android-api-9/1426824669/mozilla- > inbound_ubuntu64_vm_large_test-plain-reftest-9-bm114-tests1-linux64-build110. > txt.gz&only_show_unexpected=1 Fixed locally; the tests need to override the border-radius that the Android style sheet specifies. (In reply to Phil Ringnalda (:philor) from comment #111) > And a perhaps-intermittent failure of 359903-1.html on 10.10 debug, > https://treeherder.mozilla.org/logviewer.html#?job_id=7826496&repo=mozilla- > inbound This wasn't intermittent, and happened on both opt and debug. I'm just going to annotate the test as failing on 10.10.
Flags: needinfo?(dbaron)
Prior to the backout I'd landed a followup. https://hg.mozilla.org/integration/mozilla-inbound/rev/f33123f726b6 I'll just mark the failures on B2G as well. (I filed a bug for both sets; it's in the reftest.list.)
The new tests were disabled on B2G in bug 1145803, but hopefully it will be possible to enable them in a week or two once the chunking changes.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(In reply to howie [:howie] from comment #101) > put 2.2+ as this blocks a blocker. Since this bug blocks another 2.2+ bug, could you please uplift to 2.2? Thanks.
Comment on attachment 8578758 [details] [diff] [review] Stop allowing button contents to overflow into the CSS padding and border (although still allow them to overflow into our internal focuspadding when the min-content width says the contents don't fit) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 180085 (?), from 2002 User impact if declined: oddly-aligned text within buttons Testing completed: Examined a bunch of button testcases; reftests landed on mozilla-central. Risk to taking this patch (and alternatives if risky): some risk to Web page layout, although not huge since it's not changing the size of the button, but only the placement of the text within it String or UUID changes made by this patch: none
Attachment #8578758 - Flags: approval-mozilla-b2g37?
Comment on attachment 8578758 [details] [diff] [review] Stop allowing button contents to overflow into the CSS padding and border (although still allow them to overflow into our internal focuspadding when the min-content width says the contents don't fit) https://bugzilla.mozilla.org/show_bug.cgi?id=1135161 is verified, so approving this change to land. "If" this causes major fallout or serious regressions then we will take the gaia work around instead and probably back this out.
Attachment #8578758 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1c2dd60d2f17 I'll keep an eye out for any test-related weirdness and disable if needed like we did on m-c.
Blocks: 1160096
No longer depends on: 1160096
Issue verified fixed on Flame 2.2 and 3.0 Music file with long name is not overlapping on sound settings screen when set as default ringtone (not overlapping outside button, button is properly resized, and padding for buttons shows proper alignment.) The Default Media Location in Media Storage settings appears correctly (no "..." displayed.) Note that on 3.0 the button is vertically resized to fit text, while on 2.2 the button size remains the same and an ellipsis appears to truncate filename. Device: Flame 2.2 Build ID: 20150506002501 Gaia: 772a9491909abd02dc67278dd453746e2dd358a8 Gecko: 3af6a0a79227 Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Device: Flame 3.0 Build ID: 20150506010204 Gaia: 3e6fd1e0a478af2c95d09ce95c2c6de2de2fec14 Gecko: ba44099cbd07 Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed Version: 40.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: