Closed Bug 758079 Opened 12 years ago Closed 12 years ago

Ordered list numbers cut off on Galaxy Nexus.

Categories

(Core :: Layout, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
blocking-fennec1.0 --- .N+

People

(Reporter: bwinton, Assigned: jwir3)

References

()

Details

(Whiteboard: readability,)

Attachments

(4 files, 1 obsolete file)

As seen in https://www.dropbox.com/s/dznmcp4dsreb7bw/2012-05-23%2020.16.10.png

STR: Load the page in the url field.
     Scroll down a little ways below the first image.

Expected behaviour: I can see the entire numbers.

Observed behaviour: The numbers are cut off.  ;)

FWIW, it works fine in FF Desktop Aurora, and the default ICS browser.

Thanks,
Blake.
Attached image The misbehaviour
(And since the dropbox url didn't show up nicely with bugzilla.js, here's the same picture as an attachment.)
Reproduced on the Galaxy Nexus on recent m-c code. Turning off font inflation (settings -> text size to "tiny") fixes this. Therefore I believe it is font-inflation-related.

CC'ing dbaron.
blocking-fennec1.0: --- → ?
Hardware: x86 → All
Whiteboard: readability
(As a side note, I don't _think_ I changed the font inflation from the default, so this could hit more people than it would if I had changed that.)
Assignee: nobody → jet
blocking-fennec1.0: ? → +
The numbers in ordered lists can actually disappear at higher inflation values. As that actually hinders readability, the blocking+ should probably stick.
Assignee: jet → sjohnson
blocking-fennec1.0: + → .N+
So during the meeting we discussed the choice between the two easiest things:
 * leave things as they are
 * don't inflate list numbers (which means they'll be substantially smaller in many cases)

People thought the current situation was probably better than not inflating list numbers at all.


A real fix involves either (a) figuring out how to associate the thing that causes the indentation of a list item with the list item or (b) some heuristic that gets close enough.

It's actually probably not that hard to do (b).  In particular, we could inflate any horizontal margin and padding of anything with display:list-item and a numeric list-style-type or any HTML ul, ol, dir, or menu elements with a numeric list-style-type (or maybe just ol).
Assignee: sjohnson → nobody
Component: General → Layout
Product: Fennec Native → Core
QA Contact: general → layout
Whiteboard: readability → readability,
Assignee: nobody → sjohnson
Attached patch b758079 - WIP (obsolete) — Splinter Review
Ok, I think I have a solution for this that implements dbaron's suggested fix (b) in comment 6. I'm attaching it now. 

I have two questions about this, though - 

1) In the testcase I'm going to post, it seems that, in some cases, RTL text requires that we have a horizontal scrollbar. I'm not entirely sure why, since it *should* be able to move the RTL text a little more to the left, causing the horizontal scrollbar to disappear (since we have space). I don't know how big of a deal this is - I could continue to work with it to see what's causing this, or I could leave as-is, which, in most cases, causes RTL text that's inflated to work very similarly to LTR text. 

On the other hand, I could just remove the code for RTL text altogether, since it seems like we don't consider RTL that much in other font inflation code. Additionally, it's not like it's unreadable as in the LTR case, it's just going to need scrolling to see the bullets/list numerals/etc...

2) I used 240 as a base for margin increases, and I'd like to know if this is valid. So, what's happening right now is that the margin is increased (from the left in the case of LTR) by 240 * inflation ratio. I guess I pulled the 240 out from somewhere because I heard once that the default space we are given for list numerals is 40 css px = 40 * 60 = 240 in app units.

Another thought I had was to determine how much space the text in the bullet occupies, and then utilize this number * the inflation ration. I was worried this wouldn't be as efficient, though, so I started with this approach.
Attached file testcase-rtl.html
RTL testcase.
Also, I just noticed that I forgot to adjust the width of the container down slightly for this added margin, so I'll have to do that as well. Otherwise, the font inflation might overflow the available width.
Attached image truncated-j-screenshot
After looking at this further, I think it might extend to more situations than just lists. In the screenshot, there's a 'j' character, taken from the testcase url, that is truncated at 15 emPerLine on desktop. (I've modified the testcase a bit to make it a reduced version of the original, so it's not identical). 

This screenshot was taken without any patches applied, so it's possible that what we actually need to be doing needs to extend to more than just list numerals.
Attached patch b758079Splinter Review
Ok, I have a working patch now. I don't inflate the margins for bulleted lists, only ordered lists, since they didn't seem to be exhibiting the same problems as ordered list numerals.
Attachment #636408 - Attachment is obsolete: true
Attachment #637271 - Flags: review?(dbaron)
dbaron, review ping? We would like to get this on beta in time for the next build
Comment on attachment 637271 [details] [diff] [review]
b758079

>+/* static */ nscoord
>+nsLayoutUtils::FontSizeInflationListMarginAdjustment(const nsIFrame* aFrame) {

I think you should just make this a static function inside nsHTMLReflowState.cpp, where you use it, rather than putting in nsLayoutUtils.

Also, opening { of a function goes on its own line.

>+  const nsStyleList* styleList = aFrame->GetStyleList();
>+  float inflation = nsLayoutUtils::FontSizeInflationFor(aFrame);
>+  if (aFrame->IsFrameOfType(nsIFrame::eBlockFrame)) {
>+    const nsBlockFrame* blockFrame = static_cast<const nsBlockFrame*>(aFrame);
>+
>+    // We only want to adjust the margins if we're dealing with an ordered
>+    // list.
>+    if (blockFrame->HasBullet() &&
>+        styleList->mListStyleType != NS_STYLE_LIST_STYLE_NONE &&
>+        styleList->mListStyleType != NS_STYLE_LIST_STYLE_DISC &&
>+        styleList->mListStyleType != NS_STYLE_LIST_STYLE_CIRCLE &&
>+        styleList->mListStyleType != NS_STYLE_LIST_STYLE_SQUARE &&
>+        inflation > 1.0f) {

You should check inflation > 1.0f as the first condition of the first of these ifs to avoid making the non-inflation codepath more expensive.  You should also move the |styleList| variable inside the first if.

>+      // The HTML spec states that the default padding for ordered lists begins
>+      // at 40px, indicating that we have 40px of space to place a bullet. When
>+      // performing font inflation calculations, we add space equivalent to this,
>+      // but simply inflated at the same amount as the text, in app units.
>+      return 240 * (inflation - 1);

Why is this 240 rather than 2400?  I'd think it should be 2400, though really it should be nsPresContext::CSSPixelsToAppUnits(40).



This approach isn't great for its interaction with author styles; I'd rather have multiplied the author-specified margin/padding by the inflation than adding a multiple of the default padding.  And it also feels a little odd to add it to the margin of the item rather than the padding of the list.  That said, this approach does have the advantage that it probably works a little better when authors use things other than HTML list markup as lists.

I might prefer going with the other approach in the long term (multiplying the padding and margin of ol and of list items), but for now I think this is ok.
Attachment #637271 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #13)

I'll make the changes you requested in comment #13.

> This approach isn't great for its interaction with author styles; I'd rather
> have multiplied the author-specified margin/padding by the inflation than
> adding a multiple of the default padding.  And it also feels a little odd to
> add it to the margin of the item rather than the padding of the list.  That
> said, this approach does have the advantage that it probably works a little
> better when authors use things other than HTML list markup as lists.

Would you prefer going with this approach right now? I'm not sure how much extra work it would be, but it would ensure that we don't have to revisit this issue sometime in the future.
(In reply to Scott Johnson (:jwir3) from comment #14)
> (In reply to David Baron [:dbaron] from comment #13)
> 
> I'll make the changes you requested in comment #13.
> 
> > This approach isn't great for its interaction with author styles; I'd rather
> > have multiplied the author-specified margin/padding by the inflation than
> > adding a multiple of the default padding.  And it also feels a little odd to
> > add it to the margin of the item rather than the padding of the list.  That
> > said, this approach does have the advantage that it probably works a little
> > better when authors use things other than HTML list markup as lists.
> 
> Would you prefer going with this approach right now? I'm not sure how much
> extra work it would be, but it would ensure that we don't have to revisit
> this issue sometime in the future.

Issue is though, that 14.0.1 will be built on Monday, not sure we have time for the alternate atm.
They both have advantages and disadvantages; I'd say go with this approach for now.
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/301b36379f40
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/301b36379f40
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 637271 [details] [diff] [review]
b758079

[Approval Request Comment]
Bug caused by (feature/regressing bug #): font-inflation
User impact if declined: Ordered list numerals and bullets inflated greater than the margins on ordered and unordered lists (40px by default) will be visibly clipped.
Testing completed (on m-c, etc.): local testing, mozilla-central
Risk to taking this patch (and alternatives if risky): Fairly low risk. Changes were made to how font size inflation handles ordered and unordered lists. It now increases the margins of lists by an amount proportional to the default margin. Will only affect users who have font inflation enabled (i.e. fennec).
String or UUID changes made by this patch: None.
Attachment #637271 - Flags: approval-mozilla-aurora?
Comment on attachment 637271 [details] [diff] [review]
b758079

[Triage Comment]
This was a .N+ blocker that didn't make it into 14.0.1. Let's uplift to Aurora 15.
Attachment #637271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 773403
You need to log in before you can comment on or make changes to this bug.