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
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
Created attachment 61262 [details] Failing test case
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
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
Created attachment 98935 [details] Proposed patch
I'm confused. Perhaps it's just that the test case results are hard to read.
Created attachment 98936 [details] More simple test case
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 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
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
Created attachment 98939 [details] updated patch moved a expected result file.
(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 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
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
(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?
Created attachment 99056 [details] updated patch Add expected-result for cr-linux
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
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
Created attachment 99064 [details] updated patch
Created attachment 99068 [details] updated patch
Created attachment 112995 [details] Updated patch
I don't fully understand the context of this change. It looks reasonable. I'm not sure how RIAA wants to treat these elements?
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 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
(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
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.
I'll be very careful with respect to implementing menu item. There have been some contentions around menu/command elements.
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.