Bug 42074 - Layout problem with HTML5 menu item
Summary: Layout problem with HTML5 menu item
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://alexandre.alapetite.fr/divers/...
Keywords: HasReduction, HTML5
Depends on:
Blocks: 32934
  Show dependency treegraph
 
Reported: 2010-07-12 06:06 PDT by Alexandre Alapetite
Modified: 2011-12-14 01:19 PST (History)
10 users (show)

See Also:


Attachments
Screenshot of the padding bug in Webkit for list items in HTML5 menu (39.14 KB, image/png)
2010-07-12 06:06 PDT, Alexandre Alapetite
no flags Details
Failing test case (1.43 KB, text/html)
2010-07-12 13:26 PDT, Alexandre Alapetite
no flags Details
Proposed patch (5.91 KB, patch)
2011-06-28 10:14 PDT, ChangSeok Oh
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
More simple test case (790 bytes, text/html)
2011-06-28 10:25 PDT, ChangSeok Oh
no flags Details
Archive of layout-test-results from ec2-cr-linux-01 (1.69 MB, application/zip)
2011-06-28 10:31 PDT, WebKit Review Bot
no flags Details
updated patch (5.95 KB, patch)
2011-06-28 10:48 PDT, ChangSeok Oh
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.31 MB, application/zip)
2011-06-28 11:23 PDT, WebKit Review Bot
no flags Details
updated patch (8.86 KB, patch)
2011-06-29 02:07 PDT, ChangSeok Oh
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.41 MB, application/zip)
2011-06-29 02:28 PDT, WebKit Review Bot
no flags Details
updated patch (9.31 KB, patch)
2011-06-29 03:23 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff
updated patch (9.34 KB, patch)
2011-06-29 03:49 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Updated patch (21.70 KB, patch)
2011-10-30 10:38 PDT, ChangSeok Oh
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Alapetite 2010-07-12 06:06:45 PDT
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
Comment 1 Alexandre Alapetite 2010-07-12 13:02:40 PDT
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
Comment 2 Alexandre Alapetite 2010-07-12 13:26:28 PDT
Created attachment 61262 [details]
Failing test case
Comment 3 Alexandre Alapetite 2010-10-13 14:43:18 PDT
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
Comment 4 Alexandre Alapetite 2011-04-16 06:30:53 PDT
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
Comment 5 ChangSeok Oh 2011-06-28 10:14:55 PDT
Created attachment 98935 [details]
Proposed patch
Comment 6 Eric Seidel (no email) 2011-06-28 10:18:14 PDT
I'm confused.   Perhaps it's just that the test case results are hard to read.
Comment 7 ChangSeok Oh 2011-06-28 10:25:47 PDT
Created attachment 98936 [details]
More simple test case
Comment 8 Darin Adler 2011-06-28 10:27:05 PDT
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?
Comment 9 WebKit Review Bot 2011-06-28 10:31:36 PDT
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
Comment 10 WebKit Review Bot 2011-06-28 10:31:42 PDT
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
Comment 11 ChangSeok Oh 2011-06-28 10:48:44 PDT
Created attachment 98939 [details]
updated patch

moved a expected result file.
Comment 12 ChangSeok Oh 2011-06-28 10:51:50 PDT
(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.
Comment 13 WebKit Review Bot 2011-06-28 11:23:11 PDT
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
Comment 14 WebKit Review Bot 2011-06-28 11:23:17 PDT
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
Comment 15 ChangSeok Oh 2011-06-28 19:06:04 PDT
(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?
Comment 16 ChangSeok Oh 2011-06-29 02:07:42 PDT
Created attachment 99056 [details]
updated patch

Add expected-result for cr-linux
Comment 17 WebKit Review Bot 2011-06-29 02:27:59 PDT
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
Comment 18 WebKit Review Bot 2011-06-29 02:28:05 PDT
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
Comment 19 ChangSeok Oh 2011-06-29 03:23:59 PDT
Created attachment 99064 [details]
updated patch
Comment 20 ChangSeok Oh 2011-06-29 03:49:02 PDT
Created attachment 99068 [details]
updated patch
Comment 21 ChangSeok Oh 2011-10-30 10:38:16 PDT
Created attachment 112995 [details]
Updated patch
Comment 22 Eric Seidel (no email) 2011-12-13 23:14:28 PST
I don't fully understand the context of this change.  It looks reasonable.  I'm not sure how RIAA wants to treat these elements?
Comment 23 Eric Seidel (no email) 2011-12-13 23:15:56 PST
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?
Comment 24 Eric Seidel (no email) 2011-12-13 23:19:57 PST
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
Comment 25 Ryosuke Niwa 2011-12-13 23:37:49 PST
(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
Comment 26 Eric Seidel (no email) 2011-12-14 01:07:07 PST
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.
Comment 27 Ryosuke Niwa 2011-12-14 01:19:58 PST
I'll be very careful with respect to implementing menu item. There have been some contentions around menu/command elements.