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)
Core
Layout
Tracking
()
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
|
tif
:
ui-review-
|
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+
bajaj
:
approval-mozilla-b2g37+
|
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
ni Howie
Hi Howie, this looks like a general setting display issue, could you help to find someone to check?
Thanks
Flags: needinfo?(hochang)
Comment 3•10 years ago
|
||
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
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
Sorry, Arthur is PTO ... I should ask EJ instead.
Flags: needinfo?(arthur.chen) → needinfo?(ejchen)
Comment 7•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
(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 :)
Comment 11•10 years ago
|
||
Flagging Tif to describe the desired UX.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(tshakespeare)
Comment 12•10 years ago
|
||
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)
Updated•10 years ago
|
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!
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 17•10 years ago
|
||
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
Reporter | ||
Comment 20•10 years ago
|
||
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)
Reporter | ||
Comment 21•10 years ago
|
||
(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)
Reporter | ||
Comment 23•10 years ago
|
||
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 !!
Reporter | ||
Comment 26•10 years ago
|
||
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)
Comment 28•10 years ago
|
||
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)
Comment 32•10 years ago
|
||
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)
Comment 33•10 years ago
|
||
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)
Comment 35•10 years ago
|
||
Flagging Harly instead of Casey. Casey is not working on BB.
Flags: needinfo?(kyee) → needinfo?(hhsu)
Comment 36•10 years ago
|
||
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 :]
+++
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
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)
Comment 46•10 years ago
|
||
> 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)
Comment 47•10 years ago
|
||
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)
Comment 48•10 years ago
|
||
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"
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 :)
Comment 56•10 years ago
|
||
> `text-overflow: "... "` (<- with two more spaces) to fix this for this case
> ?
It is great!! I have updated the patch with this solution.
Comment 57•10 years ago
|
||
A few things on intl here:
Three dots isn't the right ellipsis, basically anywhere. There's a unicode glyph for …. 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 …. 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 ?
Comment 59•10 years ago
|
||
(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
Comment 60•10 years ago
|
||
(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.
Comment 61•10 years ago
|
||
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?
Comment 62•10 years ago
|
||
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.
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?
Flags: needinfo?(dbaron)
Assignee | ||
Comment 64•10 years ago
|
||
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)
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.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 68•10 years ago
|
||
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)
Assignee | ||
Comment 69•10 years ago
|
||
Conceptually that code seems to date back at least to
https://github.com/mozilla/gecko-dev/commit/ed078e5c8750a4dff9c4b16bf6110eaa152c804b (bug 180085).
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)
Updated•10 years ago
|
Mentor: ejchen
Whiteboard: [priority][mentor=ejchen][mentor-lang=zh] interaction-design → [priority][mentor-lang=zh] interaction-design
Comment 74•10 years ago
|
||
(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)
Updated•10 years ago
|
Assignee: pacorampas → nobody
David, could you please look into that again?
We have found more bugs in gaia depending on that issue.
Thanks!
Flags: needinfo?(dbaron)
Comment 76•10 years ago
|
||
noming to 2.0 as it blocks a blocker
Updated•10 years ago
|
blocking-b2g: backlog → 2.0?
Blocks: 1030631
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Comment 77•10 years ago
|
||
Hi Paco, do you mind taking this again? Somehow the assignee is removed. Thank you!
Flags: needinfo?(pacorampas)
Comment 78•10 years ago
|
||
David Baron or Jonathan Watt should take care on this as is a platform issue.
Flags: needinfo?(pacorampas)
Comment 79•10 years ago
|
||
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)
Comment 80•10 years ago
|
||
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)
Comment 81•10 years ago
|
||
Ni bbajaj so your side of people can improve the quality of triage decision by reading the bug thoroughly.
Flags: needinfo?(bbajaj)
Updated•10 years ago
|
Component: Gaia::Settings → Graphics
Flags: needinfo?(jmcf)
OS: Gonk (Firefox OS) → All
Product: Firefox OS → Core
Updated•10 years ago
|
blocking-b2g: backlog → 2.0?
Summary: [BB][settings] Truncate text in buttons and keep paddings on it → Inconsistent behavior of paddings in button element
Comment 82•10 years ago
|
||
I have changed the title of the bug and the Product and Component.
Updated•10 years ago
|
Mentor: ejchen
Component: Graphics → Layout
Whiteboard: [priority][mentor-lang=zh] interaction-design → [priority] interaction-design
Comment 83•10 years ago
|
||
> 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...
Comment 84•10 years ago
|
||
(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)
Assignee | ||
Comment 85•10 years ago
|
||
(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)
Assignee | ||
Comment 86•10 years ago
|
||
Attachment #8439261 -
Attachment is obsolete: true
Assignee | ||
Comment 87•10 years ago
|
||
Assignee | ||
Comment 88•10 years ago
|
||
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)
Updated•10 years ago
|
Flags: needinfo?(jmcf)
Comment 89•10 years ago
|
||
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
Comment 91•10 years ago
|
||
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)
Assignee | ||
Comment 93•10 years ago
|
||
(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)
Comment 94•10 years ago
|
||
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)
Assignee | ||
Comment 95•10 years ago
|
||
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.
Comment 96•10 years ago
|
||
Only by 1-2px, right? And note that consistent alignment with other browsers would mean 0 focuspadding. Something we should do anyway....
Updated•10 years ago
|
Comment 98•10 years ago
|
||
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
Assignee | ||
Comment 99•10 years ago
|
||
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.)
Assignee | ||
Comment 102•10 years ago
|
||
Revised patch at https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/f94852c33369/button-dont-erode-padding ; still need to test.
Assignee | ||
Comment 103•10 years ago
|
||
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
Assignee | ||
Comment 104•10 years ago
|
||
Comment 105•10 years ago
|
||
Hi David, I'd put you as assignee if you don't mind, thanks!
Assignee: nobody → dbaron
Assignee | ||
Comment 106•10 years ago
|
||
(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.)
Assignee | ||
Comment 107•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8456571 -
Attachment is obsolete: true
Updated•10 years ago
|
Hardware: ARM → All
Version: unspecified → Trunk
Comment 108•10 years ago
|
||
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+
Assignee | ||
Comment 109•10 years ago
|
||
Comment 110•10 years ago
|
||
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
Flags: needinfo?(dbaron)
Comment 111•10 years ago
|
||
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
Assignee | ||
Comment 112•10 years ago
|
||
(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)
Assignee | ||
Comment 113•10 years ago
|
||
Comment 114•10 years ago
|
||
Backed out for B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be9842d5121
https://treeherder.mozilla.org/logviewer.html#?job_id=7846384&repo=mozilla-inbound
Assignee | ||
Comment 115•10 years ago
|
||
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.)
Assignee | ||
Comment 116•10 years ago
|
||
Assignee | ||
Comment 117•10 years ago
|
||
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.
Comment 118•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 119•10 years ago
|
||
(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.
Assignee | ||
Comment 120•10 years ago
|
||
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 122•10 years ago
|
||
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+
Comment 123•10 years ago
|
||
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.
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox37:
--- → wontfix
status-firefox38:
--- → wontfix
Updated•10 years ago
|
Comment 124•10 years ago
|
||
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)
Updated•10 years ago
|
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.
Description
•