Closed Bug 964900 Opened 10 years ago Closed 10 years ago

<li> element cut off on left side if text zoomed in

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox27 --- unaffected
firefox28 - affected
firefox29 - affected

People

(Reporter: alice0775, Assigned: xidorn)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached file testcase
Steps To Reproduce:
1. Open attached
2. Alt > View > Zoom > select "Zoom Text Only"
3. Zoom in several times (Press Ctrl++ several times)

Actual Results:
<li> element cut off on left side 

Expected Results:
Should not cut off


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea87ed77256e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131202095056
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/07e357311cf3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131202095303
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ea87ed77256e&tochange=07e357311cf3

Regressed by: Bug 934072
Attached image screenshot
This also happens on ubuntu12.04.
OS: Windows 7 → All
Assignee: nobody → quanxunzhen
Hardware: x86_64 → All
Version: 28 Branch → Trunk
I'm not seeing this issue in Nightly on either OS X or Windows. Maybe it could be related to Japanese-locale font settings. (So perhaps it'll also show up for Chinese users?)
Did you switch to "Zoom Text Only"?

This is related to the indentation of lists being hardcoded as 40px.  This matches http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#lists , although I suppose it's possible we could change it to 2.5em and still be Web-compatible.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #4)
> Did you switch to "Zoom Text Only"?
> 
> This is related to the indentation of lists being hardcoded as 40px.  This
> matches
> http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.
> html#lists , although I suppose it's possible we could change it to 2.5em
> and still be Web-compatible.

Do you mean, this is the expected result from the current spec? Maybe we should propose a change on the spec as well as our code, since it is not a good idea to hardcode in pixel for most time.
Flags: needinfo?(dbaron)
This is the expected result from the spec, which says what it does since that's what browsers have done for many years.  Changing the behavior might cause compatibility problems, or it might not.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #6)
> This is the expected result from the spec, which says what it does since
> that's what browsers have done for many years.  Changing the behavior might
> cause compatibility problems, or it might not.

So do you think we should mark this bug as WONTFIX or change the default indentation? Changing padding-start to 2.5em indeed fixes this bug. I just wonder whether we should do this change.
I think 2.5em would be a far more sensible setting. (Just try changing the root font size to something large like 72px with the current setting, and see how poorly it works.)

Personally, I'd be inclined to say we should experiment with this change, and then see whether web-compat problems crop up as it moves through Nightly, Aurora and Beta. It should be a simple change, and trivial to revert if we do see significant problems, but if we can successfully deploy it I think it's fundamentally better.
:dbaron, would you be OK with us landing a change from 40px to 2.5em in Nightly, and see how things go, or does it strike you as too risky?
Flags: needinfo?(dbaron)
Attached patch patchSplinter Review
This super simple patch could fix this problem, but may break the spec. I'm not sure whether it is properly to apply such patch. Maybe we shouldn't do that since the fixed value is specified by the spec, while Zoom Text Only is not a part of specs.
Could you explain why bug 934072 regressed this?
Flags: needinfo?(quanxunzhen)
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> :dbaron, would you be OK with us landing a change from 40px to 2.5em in
> Nightly, and see how things go, or does it strike you as too risky?

I'm ok with trying it in Nightly; I'm not ok with backporting it to branches.

I'd like to hear the answer to comment 11 before suggesting what we should do about branches.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #11)
> Could you explain why bug 934072 regressed this?

It is because in the first patch (attachment 8340758 [details] [diff] [review]), I changed the spacing between marker and content from 8px to 0.5em. Jonathan and I both agree that it is a better value since it links the spacing to the font-size which can provide a clearer effect when text gets larger. (Behavior in WebKit is similar, though they use a space character instead of 0.5em)

I know this change seems to be unrelated with bug 934072. We included it since the two values are almost equal by default, and we thought it was a trivial change.
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan from comment #13)
> It is because in the first patch (attachment 8340758 [details] [diff] [review]
> [review]), I changed the spacing between marker and content from 8px to
> 0.5em. Jonathan and I both agree that it is a better value since it links
> the spacing to the font-size which can provide a clearer effect when text
> gets larger. (Behavior in WebKit is similar, though they use a space
> character instead of 0.5em)

If we think this bug is serious enough to need to fix on branches (I'm not sure), then I think we should back that part out on the branches, and try the 2.5em patch on trunk.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> If we think this bug is serious enough to need to fix on branches (I'm not
> sure), then I think we should back that part out on the branches, and try
> the 2.5em patch on trunk.

I turn to disagree with applying this patch. IMHO we shouldn't touch the value specified by the spec, since it could introduce unwanted compatible problems among browsers.

I think it might be better to mark this bug as wontfix.
See what will happen if the padding is changed from 40px to 2.5em.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> (In reply to Xidorn Quan from comment #13)
> > It is because in the first patch (attachment 8340758 [details] [diff] [review]
> > [review]), I changed the spacing between marker and content from 8px to
> > 0.5em. Jonathan and I both agree that it is a better value since it links
> > the spacing to the font-size which can provide a clearer effect when text
> > gets larger. (Behavior in WebKit is similar, though they use a space
> > character instead of 0.5em)
> 
> If we think this bug is serious enough to need to fix on branches (I'm not
> sure), 

I don't think it is - especially given how similar Webkit's behavior is, which makes it seem unlikely there are major web-compat issues with that aspect. An author using <li> with huge font sizes will need to be overriding some defaults for padding/margins/etc anyhow in order to get reasonable-looking results across browsers.

> then I think we should back that part out on the branches, and try
> the 2.5em patch on trunk.

...so IMO, it would be OK to simply WONTFIX this. But if we -could- make the padding change here (and get it adopted as an official spec change?), I think it'd be much more sensible default behavior, and as such it'd be an improvement to the web platform.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: