Rework list marker spacing for better web compatibility.

Rework list marker spacing for better web compatibility.

- Revise spacing between list marker and list item text to position marker
  directly alongside text. This essentially leads to rendering the marker text a
  bit farther to the right (for LTR) or left (for RTL), which is more compatible
  with what we see in IE/FF. Previously we applied varying logic that led to a
  larger space, typically ~several font-space-width in width, between marker and
  item text. Our deviance from other browser behavior is what led to the
  associated bug wherein web developer was surprised that, only in Chrome,
  marker text was cut off by overflow: hidden.

- Only highlight selected list markers when
  SelectionPaintingWithoutSelectionGaps is disabled. Note even prior to this
  change the text of the first list marker could never be selected.

- Introduce implementation-internal notion of list style 'category' to reduce
  redundant verbose switch statements.

- Render 'symbol' list style (disc, square, circle) markers by just rendering
  their associated marker text character. Previously we painted the symbols with
  appropriate graphics calls. This leads to a slightly different look for the
  symbol bullets (in default font, for example, smaller). This simplifies layout
  and paint logic.

BUG=472084

[wkorman, 2015-10-10 06:20:13]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-10 06:20:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403643002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403643002/1

[commit-bot: I haz the power, 2015-10-10 08:10:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-10 08:10:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2015-10-12 02:49:58]

wkorman@chromium.org changed reviewers:
+ eae@chromium.org, leviw@chromium.org

[wkorman, 2015-10-12 02:49:58]

Seeking initial feedback. I want to make sure I'm not doing anything
fundamentally untoward. Still in work are:

- reviewing image lists and vertical writing mode. Vertical looks fine but I've
not reviewed rigorously yet. Image lists I've not looked at at all. I expect to
revise their layout further in subsequent change as this is already large.

- updating included tests to attempt as ref tests, and include
image/vertical/vertical-rtl variants in the meta-test I've added for
thoroughness and easier review of the many permutations of list styles
available.

- updating test expectations to incorporate what's needed from dry run.

In hindsight this could be broken into smaller changes if desired as:

- minor syntax nit in LayoutListItem.{h,cpp}
- introduce simplified list style to reduce verbose switch statements
- flag off selection highlighting of marker items
- stop painting the symbol (disc, square, circle) via graphics calls, just
render as text
- change spacing/positioning logic

[wkorman, 2015-10-12 03:00:40]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-12 03:00:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403643002/20002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403643002/20002

[commit-bot: I haz the power, 2015-10-12 03:12:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-12 03:12:23]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[wkorman, 2015-10-12 03:33:32]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-12 03:33:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403643002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403643002/70001

[commit-bot: I haz the power, 2015-10-12 04:38:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-12 04:38:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[eae, 2015-10-12 06:02:01]

I like it!

If we use text to draw the list markers it should be possible to use ref-tests
that use list-position inside and list-style none and then have the list marker
as pure text.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListItem.cpp:63:
notifyOfSubtreeChange();
Can this cause style()->listStyleImage() to change? If so we need to handle
that, if not please add an assert.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:42: const int
cMarkerPadding = 7;
cMarkerPaddingPx (assuming it is in pixels).

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:1178: int bulletWidth
= style()->fontMetrics().ascent() / 2;
Is flooring the result the tight thing to do here? If so please make it explicit
by adding a floorf call.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:1204: TextRun run =
constructTextRun(font, suffix, suffixLength, styleRef(), style()->direction());
How about constructing a text run with both the text and the suffix? Should make
bidi handling easaier. Obviously that'll require that we paint them together
too...

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListMarker.h (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.h:52: SimplifiedListStyle
simplifiedListStyle() const;
listStyleCategory might be a better name as simplified and list-style both have
very specific meanings.

[wkorman, 2015-10-14 01:55:31]

Finishing up test expectation rebaseline review now. Seeking your
feedback on a few tests, reference these results from patch 6 (not all
tests unless you're zealous to validate, just the ones I mention
below):

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

I think these look ok, after going through ~240 affected
rebaselines. But I'm fine with reverting the portion of this change
that switches to text for the symbols if you'd prefer.

- for list-style-position: inside the bullet now renders farther over
  toward the text, consistent with how it normally renders. This looks
  more surprisingly different than one would expect, though, when the
  list item content wraps to the next line. See samples:

  fast/css/001.html
  css1/classification/list_style_position.html

  I played around with alternate positioning but I think if we want to
  do more here it will require changes that are better done as
  subsequent revision. We may want to separate some
  positioning/dimensions that are today conjoined in the logic,
  i.e. size vs paint position.

- rendering disc/square/bullet via font leads to very different
  visuals, see:

  fast/lists/list-marker-before-content-table.html

  Also note the vertical spacing in the ::before portion of this test
  has changed, likely due to font metrics, but could be another bug.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListItem.cpp:59: || (currentImage &&
!currentImage->errorOccurred())) {
codereview UI isn't showing your comment in-line here for some reason -- re:
whether this can cause list style image to change, I believe yes, which would be
handled by the bit below (line 69+), but I may be missing something, let me
know.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:42: const int
cMarkerPadding = 7;
On 2015/10/12 at 06:02:01, eae wrote:
> cMarkerPaddingPx (assuming it is in pixels).

Done.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:1178: int bulletWidth
= style()->fontMetrics().ascent() / 2;
On 2015/10/12 at 06:02:01, eae wrote:
> Is flooring the result the tight thing to do here? If so please make it
explicit by adding a floorf call.

Likely to revise this in subsequent changes when I review image lists in more
detail. This preserves existing behavior for now.

Is floorf the right thing to use with ints? ascent is an int as well so it's
implicitly floored/truncated, I also prefer being explicit, just not sure what
Blink style is here.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:1204: TextRun run =
constructTextRun(font, suffix, suffixLength, styleRef(), style()->direction());
On 2015/10/12 at 06:02:01, eae wrote:
> How about constructing a text run with both the text and the suffix? Should
make bidi handling easaier. Obviously that'll require that we paint them
together too...

Added TODO to look at this, I agree there's redundant logic this could clean up.

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutListMarker.h (right):

https://codereview.chromium.org/1403643002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListMarker.h:52: SimplifiedListStyle
simplifiedListStyle() const;
On 2015/10/12 at 06:02:01, eae wrote:
> listStyleCategory might be a better name as simplified and list-style both
have very specific meanings.

Done.

[wkorman, 2015-10-14 05:44:02]

I'm breaking this apart into smaller changes, several already mailed to eae@.
Will explore keeping the painted symbols. It seems pretty deviant to abandon
them now that I've looked more closely at FF behavior. If you feel strongly
otherwise let me know. See my previous note on this change for more detail.