Bug 42074 – Layout problem with HTML5 menu item

• Status: UNCONFIRMED
• Component: Layout and Rendering

[Alexandre Alapetite]

Created attachment 61216 [details]
Screenshot of the padding bug in Webkit for list items in HTML5 menu

Hello,
In HTML5, not only UL and OL elements can contain some LI, but also the new MENU element.
The styling of LI inside a MENU is buggy, possibly due to a padding-start value that cannot be overridden.
In the case of MENU, there is an additional unwanted margin that cannot be overridden by setting a margin, padding, or even -webkit-padding-start to zero.

See test case:
http://alexandre.alapetite.fr/divers/vrac/20100712-HTML5-menu-li.html

Problem confirmed on 2010-07-12 with Webkit SVN-r62608, Safari 5.0.7533.16, Chrome 6.0.458.1.

No problem in Opera 10.60, Firefox 3.6.6, Minefield 4.0b2pre 64-bit, Internet Explorer 9.0.7874.6000 (and IE 5-8 with proper HTML5 hack).

Please note that replacing MENU by a DIV gives the exact same rendering.

Best regards,
Alexandre
http://alexandre.alapetite.fr

[Alexandre Alapetite]

Problem confirmed for older versions of WebKit and other platforms:
For instance, buggy on Safari 3.2.3 (Mac OS X 10.5), Safari 4 (XP), Chrome 5.0.375.99 (XP), Chrome 5.0.375.99 (Ubuntu 8.04), Konqueror 3.5 and 4.2 (Debian testing), Epiphany 2.30.2 (Ubuntu 9.10).

The test case still passes fine on all other (non-WebKit)-browsers and platforms (e.g. Opera, Firefox, IE, on Windows, Linux, BSD, MacOS).

More on
http://browsershots.org/http://alexandre.alapetite.fr/divers/vrac/20100712-HTML5-menu-li.html

Regards,
Alexandre
http://alexandre.alapetite.fr

[Alexandre Alapetite]

Created attachment 61262 [details]
Failing test case

[Alexandre Alapetite]

Hello,
The problem is still there: tested today with WebKit-SVN-r69653 and Chrome 8.0.552.0-dev, under Windows 7 and Windows XP.
Could someone confirm this bug?
Cordially,
Alexandre
http://alexandre.alapetite.fr

[Alexandre Alapetite]

Hello,
This is my last attempt to get a confirmation.
More than 9 months later, the bug is still there with an unchanged status.

- Bug visible today in WebKit-SVN-r84069, Safari 5, and Chrome 12.0.725.0 dev-m (that is 6 major versions later...)

- No problem in Opera 11.10, Firefox 4.0, Internet Explorer 10.0pp1.
http://alexandre.alapetite.fr/divers/vrac/20100712-HTML5-menu-li.html

Cordially,
Alexandre Alapetite
http://alexandre.alapetite.fr

[ChangSeok Oh]

Created attachment 98935 [details]
Proposed patch

[Eric Seidel (no email)]

I'm confused.   Perhaps it's just that the test case results are hard to read.

[ChangSeok Oh]

Created attachment 98936 [details]
More simple test case

[Darin Adler]

Comment on attachment 98935 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98935&action=review

> Source/WebCore/html/HTMLLIElement.cpp:103
> -            if (n->hasTagName(ulTag) || n->hasTagName(olTag))
> +            if (n->hasTagName(ulTag) || n->hasTagName(olTag) || n->hasTagName(menuTag))

This change seems OK, but I am concerned that there are many other places that handle ul and ol, but not menu. I’d prefer to fix more of them.

My search show cases of this in these source files. I suspect many of these need the same fix:

accessibility/AccessibilityList.cpp
accessibility/AccessibilityRenderObject.cpp
accessibility/AXObjectCache.cpp
editing/CompositeEditCommand.cpp
editing/DeleteButtonController.cpp
editing/Editor.cpp
editing/htmlediting.cpp
editing/IndentOutdentCommand.cpp
editing/InsertListCommand.cpp
editing/markup.cpp
editing/TextIterator.cpp
rendering/RenderCounter.cpp
rendering/RenderListItem.cpp

Maybe we need a helper function to use in these places?

[WebKit Review Bot]

Comment on attachment 98935 [details]
Proposed patch

Attachment 98935 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8944908

New failing tests:
fast/lists/menu-list-style-position.html

[WebKit Review Bot]

Created attachment 98938 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[ChangSeok Oh]

Created attachment 98939 [details]
updated patch

moved a expected result file.

[ChangSeok Oh]

(In reply to comment #6)
> I'm confused.   Perhaps it's just that the test case results are hard to read.

There are two groups, 'Outsie' & 'Inside' in attached test case.
They mean css style value for list-style-position.
Three items of each group should align vertically.

[WebKit Review Bot]

Comment on attachment 98939 [details]
updated patch

Attachment 98939 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8944923

New failing tests:
fast/lists/menu-list-style-position.html

[WebKit Review Bot]

Created attachment 98948 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[ChangSeok Oh]

(In reply to comment #8)
> (From update of attachment 98935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98935&action=review
> 
> > Source/WebCore/html/HTMLLIElement.cpp:103
> > -            if (n->hasTagName(ulTag) || n->hasTagName(olTag))
> > +            if (n->hasTagName(ulTag) || n->hasTagName(olTag) || n->hasTagName(menuTag))
> 
> This change seems OK, but I am concerned that there are many other places that handle ul and ol, but not menu. I’d prefer to fix more of them.
> 
> My search show cases of this in these source files. I suspect many of these need the same fix:
> 
> accessibility/AccessibilityList.cpp
> accessibility/AccessibilityRenderObject.cpp
> accessibility/AXObjectCache.cpp
> editing/CompositeEditCommand.cpp
> editing/DeleteButtonController.cpp
> editing/Editor.cpp
> editing/htmlediting.cpp
> editing/IndentOutdentCommand.cpp
> editing/InsertListCommand.cpp
> editing/markup.cpp
> editing/TextIterator.cpp
> rendering/RenderCounter.cpp
> rendering/RenderListItem.cpp
> 
> Maybe we need a helper function to use in these places?

I think your opinion is usually true, but I can't believe these all files affect this layout issue.
How about dealing with it in a new bug?

[ChangSeok Oh]

Created attachment 99056 [details]
updated patch

Add expected-result for cr-linux

[WebKit Review Bot]

Comment on attachment 99056 [details]
updated patch

Attachment 99056 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8959137

New failing tests:
fast/lists/menu-list-style-position.html

[WebKit Review Bot]

Created attachment 99058 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[ChangSeok Oh]

Created attachment 99064 [details]
updated patch

[ChangSeok Oh]

Created attachment 99068 [details]
updated patch

[ChangSeok Oh]

Created attachment 112995 [details]
Updated patch

[Eric Seidel (no email)]

I don't fully understand the context of this change.  It looks reasonable.  I'm not sure how RIAA wants to treat these elements?

[Eric Seidel (no email)]

http://www.whatwg.org/specs/web-apps/current-work/#the-menu-element is the relevant part of the spec.

Do you know who originally implemented <menu> in WebKit? Are they CC'd already?

[Eric Seidel (no email)]

Comment on attachment 112995 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112995&action=review

Although darin encouraged you to fix more of these cases, your testing is insufficient for all these changes.  Either you should only make the changes you are testing and file bugs about the other cases, or you should test all the cases you're changing in this patch. :)  I suspect you should start with the first approach.  Alternatively, you could add FIXME's to the places you believe need these checks but are not yet tested.

> Source/WebCore/html/parser/HTMLElementStack.cpp:87
>  inline bool isListItemScopeMarker(ContainerNode* node)
>  {
>      return isScopeMarker(node)
> +        || node->hasTagName(menuTag)
>          || node->hasTagName(olTag)
>          || node->hasTagName(ulTag);
>  }

This function is directly derived from the HTML5 parsing spec.  It should be trivial to confirm if menu is a scope marker or not.

I do not see it listed here:
http://www.whatwg.org/specs/web-apps/current-work/#has-an-element-in-scope

[Ryosuke Niwa]

(In reply to comment #23)
> http://www.whatwg.org/specs/web-apps/current-work/#the-menu-element is the relevant part of the spec.
> 
> Do you know who originally implemented <menu> in WebKit? Are they CC'd already?

We don't implement HTML5's menu element. What we implement is the one inherited from HTML3.x, which had been deprecated in HTML4.01: http://www.w3.org/TR/html401/struct/lists.html#edef-MENU

[Eric Seidel (no email)]

So it seems that this patch is a bit premature. :)  Seems we need a bug for implementing HTML5 menu, and this should be blocked by that.

[Ryosuke Niwa]

I'll be very careful with respect to implementing menu item. There have been some contentions around menu/command elements.

[Anne van Kesteren]

This still seems like a valid problem, at least as far as treating <menu> as a list container rendering-wise goes.

Chromium has the same issue though. Only Gecko renders the test as expected.