Alice0775 White Reporter
	Description • 10 years ago

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

	Alice0775 White Reporter
	Comment 2 • 10 years ago

This also happens on ubuntu12.04.

	Jonathan Kew [:jfkthame]
	Comment 3 • 10 years ago

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

	David Baron :dbaron:
	Comment 4 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 10 years ago

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

	David Baron :dbaron:
	Comment 6 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 10 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 8 • 10 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 9 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 10 • 10 years ago

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.

	David Baron :dbaron:
	Comment 11 • 10 years ago

Could you explain why bug 934072 regressed this?

	David Baron :dbaron:
	Comment 12 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 13 • 10 years ago

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

	David Baron :dbaron:
	Comment 14 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 15 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 16 • 10 years ago

See what will happen if the padding is changed from 40px to 2.5em.

	Jonathan Kew [:jfkthame]
	Comment 17 • 10 years ago

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