Consistently autosize markers in <ul> and <ol> lists.

Consistently autosize markers and layout in <ul> and <ol> lists.

This patch implements a 'global' approach where the whole list is autosized consistently and such that it remains readable after autosizing. The fix yields correct marker autosizing in cases where there are empty list items, list items containing only images and list items with form input fields.
+ Added relevant tests

BUG=248190, 249386
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152905

[timvolodine, 2013-06-18 17:44:17]

Hi Julien,
could you have a look at this CL?

thanks,
Tim

[Julien - ping for review, 2013-06-20 16:11:51]

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
File Source/core/rendering/TextAutosizer.cpp (right):

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:97: while (ancestor) {
Nit: This really like a for-loop disguised as a while.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:110: if (child->isListItem()) {
We prefer early return to avoid having a lot of nested levels.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:254: if (RenderObject* list =
getAncestorList(listItemAncestor)) {
Don't you expect a listItem to have an enclosing list?

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:286: && (parentNode->hasTagName(olTag)
|| parentNode->hasTagName(ulTag)));
A logical AND in an ASSERT usually means that it should be split in 2. Here:

#ifndef NDEBUG
Node* parentNode = renderer->generatingNode();
ASSERT(parentNode);
ASSERT(parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag));
#endif

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:289:
newListStyle->setMarginStart(Length(newListStyle->marginStart().value() +
adjustedMargin, Fixed));
Touching the style is not a good idea as it is visible to JS. Why can't you just
modify the computed margin instead?

Also this is completely wrong: if marginStart() is not an absolute value (px,
cm, ...), you are doing arithmetic between different type. An example is that
margin-top: 50% gets converted to 50px (marginTop().value() is 50) in your
computation.

[timvolodine, 2013-06-21 15:22:32]

I've removed the margin modifying code for now.

Julien, could you have another look please?

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
File Source/core/rendering/TextAutosizer.cpp (right):

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:97: while (ancestor) {
On 2013/06/20 16:11:51, Julien Chaffraix wrote:
> Nit: This really like a for-loop disguised as a while.

Done.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:110: if (child->isListItem()) {
On 2013/06/20 16:11:51, Julien Chaffraix wrote:
> We prefer early return to avoid having a lot of nested levels.

Done.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:254: if (RenderObject* list =
getAncestorList(listItemAncestor)) {
On 2013/06/20 16:11:51, Julien Chaffraix wrote:
> Don't you expect a listItem to have an enclosing list?

what if the html is broken? even in that case autosizer will not crash.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:286: && (parentNode->hasTagName(olTag)
|| parentNode->hasTagName(ulTag)));
On 2013/06/20 16:11:51, Julien Chaffraix wrote:
> A logical AND in an ASSERT usually means that it should be split in 2. Here:
> 
> #ifndef NDEBUG
> Node* parentNode = renderer->generatingNode();
> ASSERT(parentNode);
> ASSERT(parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag));
> #endif

Done.

https://codereview.chromium.org/17071009/diff/1/Source/core/rendering/TextAut...
Source/core/rendering/TextAutosizer.cpp:289:
newListStyle->setMarginStart(Length(newListStyle->marginStart().value() +
adjustedMargin, Fixed));
On 2013/06/20 16:11:51, Julien Chaffraix wrote:
> Touching the style is not a good idea as it is visible to JS. Why can't you
just
> modify the computed margin instead?
> 
> Also this is completely wrong: if marginStart() is not an absolute value (px,
> cm, ...), you are doing arithmetic between different type. An example is that
> margin-top: 50% gets converted to 50px (marginTop().value() is 50) in your
> computation.

ok, I've removed this for now. To be addressed in a separate patch at the
render-block level as discussed in chat yesterday.

[Julien - ping for review, 2013-06-21 19:01:53]

lgtm

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
File Source/core/rendering/TextAutosizer.cpp (right):

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
Source/core/rendering/TextAutosizer.cpp:97: if (parentNode &&
(parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag)))
HTML5 allows <menu> in the toolbar state for the context in which to see li
(http://www.whatwg.org/specs/web-apps/current-work/multipage/grouping-content....).
I don't think we support that but it's definitely worth a FIXME about it.

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
Source/core/rendering/TextAutosizer.cpp:269:
renderer->setStyle(newListStyle.release());
It seems that this is just setMultiplier() above.

I don't think doing a RenderStyle::clone() is doing what we want and could trip
over style sharing. I won't block the patch as this was already wrong but please
follow-up on that.

[timvolodine, 2013-06-21 19:21:37]

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
File Source/core/rendering/TextAutosizer.cpp (right):

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
Source/core/rendering/TextAutosizer.cpp:97: if (parentNode &&
(parentNode->hasTagName(olTag) || parentNode->hasTagName(ulTag)))
On 2013/06/21 19:01:53, Julien Chaffraix wrote:
> HTML5 allows <menu> in the toolbar state for the context in which to see li
>
(http://www.whatwg.org/specs/web-apps/current-work/multipage/grouping-content....).
> I don't think we support that but it's definitely worth a FIXME about it.

Done.

https://chromiumcodereview.appspot.com/17071009/diff/7001/Source/core/renderi...
Source/core/rendering/TextAutosizer.cpp:269:
renderer->setStyle(newListStyle.release());
On 2013/06/21 19:01:53, Julien Chaffraix wrote:
> It seems that this is just setMultiplier() above.
> 
> I don't think doing a RenderStyle::clone() is doing what we want and could
trip
> over style sharing. I won't block the patch as this was already wrong but
please
> follow-up on that.

Done.

[commit-bot: I haz the power, 2013-06-21 19:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/17071009/11001

[commit-bot: I haz the power, 2013-06-21 20:36:56]

Change committed as 152905