Last Comment Bug 758079 - Ordered list numbers cut off on Galaxy Nexus.
: Ordered list numbers cut off on Galaxy Nexus.
Status: RESOLVED FIXED
readability,
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla16
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
http://higherorderfun.com/blog/2012/0...
: 760969 (view as bug list)
Depends on: 773403
Blocks: font-inflation
  Show dependency treegraph
 
Reported: 2012-05-23 17:26 PDT by Blake Winton (:bwinton) (:☕️)
Modified: 2012-07-12 14:35 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.N+


Attachments
The misbehaviour (219.08 KB, image/png)
2012-05-23 17:30 PDT, Blake Winton (:bwinton) (:☕️)
no flags Details
b758079 - WIP (11.45 KB, patch)
2012-06-25 11:29 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Review
testcase-rtl.html (187 bytes, text/html)
2012-06-25 11:29 PDT, Scott Johnson (:jwir3)
no flags Details
truncated-j-screenshot (148.65 KB, image/png)
2012-06-25 16:06 PDT, Scott Johnson (:jwir3)
no flags Details
b758079 (12.24 KB, patch)
2012-06-27 14:49 PDT, Scott Johnson (:jwir3)
dbaron: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Blake Winton (:bwinton) (:☕️) 2012-05-23 17:26:23 PDT
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.
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-05-23 17:30:25 PDT
Created attachment 626649 [details]
The misbehaviour

(And since the dropbox url didn't show up nicely with bugzilla.js, here's the same picture as an attachment.)
Comment 2 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-23 19:39:20 PDT
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.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-05-24 07:28:52 PDT
(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.)
Comment 4 Jet Villegas (:jet) 2012-05-29 15:02:10 PDT
The numbers in ordered lists can actually disappear at higher inflation values. As that actually hinders readability, the blocking+ should probably stick.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-03 09:48:52 PDT
*** Bug 760969 has been marked as a duplicate of this bug. ***
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-04 17:13:33 PDT
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).
Comment 7 Scott Johnson (:jwir3) 2012-06-25 11:29:32 PDT
Created attachment 636408 [details] [diff] [review]
b758079 - WIP

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.
Comment 8 Scott Johnson (:jwir3) 2012-06-25 11:29:58 PDT
Created attachment 636410 [details]
testcase-rtl.html

RTL testcase.
Comment 9 Scott Johnson (:jwir3) 2012-06-25 11:31:34 PDT
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.
Comment 10 Scott Johnson (:jwir3) 2012-06-25 16:06:33 PDT
Created attachment 636520 [details]
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.
Comment 11 Scott Johnson (:jwir3) 2012-06-27 14:49:03 PDT
Created attachment 637271 [details] [diff] [review]
b758079

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.
Comment 12 Brad Lassey [:blassey] (use needinfo?) 2012-07-03 11:54:45 PDT
dbaron, review ping? We would like to get this on beta in time for the next build
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-05 13:45:23 PDT
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.
Comment 14 Scott Johnson (:jwir3) 2012-07-06 07:49:16 PDT
(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.
Comment 15 JP Rosevear [:jpr] 2012-07-08 16:32:04 PDT
(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.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-08 16:41:10 PDT
They both have advantages and disadvantages; I'd say go with this approach for now.
Comment 17 Scott Johnson (:jwir3) 2012-07-10 09:04:00 PDT
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/301b36379f40
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-07-10 20:33:46 PDT
https://hg.mozilla.org/mozilla-central/rev/301b36379f40
Comment 19 Scott Johnson (:jwir3) 2012-07-12 10:46:39 PDT
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.
Comment 20 Alex Keybl [:akeybl] 2012-07-12 13:57:30 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.