Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(201)

Issue 786753003: Implement type=radio and radiogroup attribute for menuitem. (Closed)

Created:
6 years ago by pals
Modified:
6 years ago
Reviewers:
tkent
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, arv+blink, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Implement type=radio and radiogroup attribute for menuitem. The radiogroup attribute gives the name of the group of commands that will be toggled when the command itself is toggled, for commands whose type attribute has the value "radio". The scope of the name is the child list of the parent element. The attribute must be omitted unless the type attribute is in the Radio state. When specified, the attribute's value must be a non-empty string. If the type attribute is in the Radio state If the element has a parent, then the UA must walk the list of child nodes of that parent element, and for each node that is a menuitem element, if that element has a radiogroup attribute whose value exactly matches the current element's (treating missing radiogroup attributes as if they were the empty string), and has a checked attribute, must remove that attribute. Then, the element's checked attribute must be set to the literal value "checked". Specification https://html.spec.whatwg.org/multipage/forms.html#attr-menuitem-radiogroup BUG=87553 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186848

Patch Set 1 #

Patch Set 2 : Tests #

Total comments: 4

Patch Set 3 : Fixed review comments and added more tests #

Total comments: 8

Patch Set 4 : Changes as per specificatins #

Patch Set 5 : Rebaselined #

Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -5 lines) Patch
M LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html View 1 3 chunks +8 lines, -1 line 0 comments Download
M LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu-expected.txt View 1 2 chunks +4 lines, -1 line 0 comments Download
M LayoutTests/fast/dom/HTMLMenuItemElement/menuitem-click.html View 1 2 3 2 chunks +58 lines, -0 lines 0 comments Download
M LayoutTests/fast/dom/HTMLMenuItemElement/menuitem-click-expected.txt View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
M LayoutTests/webexposed/element-instance-property-listing-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLAttributeNames.in View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLMenuItemElement.cpp View 1 2 3 2 chunks +13 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMenuItemElement.idl View 1 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/page/ContextMenuControllerTest.cpp View 1 3 chunks +19 lines, -1 line 0 comments Download
M Source/core/page/CustomContextMenuProvider.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 27 (10 generated)
pals
Please review.
6 years ago (2014-12-09 11:44:29 UTC) #4
tkent
https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right): https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html#newcode200 LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:200: </script> Needs more test scenarios. * Clicking a menuitem ...
6 years ago (2014-12-09 12:43:43 UTC) #5
tkent
https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMenuItemElement.cpp File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/786753003/diff/60001/Source/core/html/HTMLMenuItemElement.cpp#newcode33 Source/core/html/HTMLMenuItemElement.cpp:33: while (nextElement) { for (HTMLMenuItemElement& menuItem : Traversal<HTMLMenuItemElement>::childrenOf(*parent)) { ...
6 years ago (2014-12-09 13:02:26 UTC) #6
tkent
https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right): https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html#newcode200 LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html:200: </script> On 2014/12/09 12:43:43, tkent wrote: > Needs more ...
6 years ago (2014-12-09 13:12:09 UTC) #7
pals
On 2014/12/09 12:43:43, tkent wrote: > https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html > File LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html (right): > > https://codereview.chromium.org/786753003/diff/60001/LayoutTests/fast/dom/HTMLMenuElement/custom-context-menu.html#newcode200 > ...
6 years ago (2014-12-09 14:48:49 UTC) #8
tkent
On 2014/12/09 14:48:49, sanjoy_pal wrote: > <menu> > <menuitem id=mi1 type=radio radiogroup="g1" checked> > <menu> ...
6 years ago (2014-12-09 21:26:17 UTC) #9
pals
On 2014/12/09 21:26:17, tkent wrote: > On 2014/12/09 14:48:49, sanjoy_pal wrote: > > <menu> > ...
6 years ago (2014-12-10 06:24:33 UTC) #10
tkent
Please follow the specification. https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMenuItemElement.cpp File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMenuItemElement.cpp#newcode31 Source/core/html/HTMLMenuItemElement.cpp:31: String group = fastHasAttribute(radiogroupAttr) ? ...
6 years ago (2014-12-10 06:50:49 UTC) #11
pals
Updated the code as per specification. https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMenuItemElement.cpp File Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/786753003/diff/80001/Source/core/html/HTMLMenuItemElement.cpp#newcode31 Source/core/html/HTMLMenuItemElement.cpp:31: String group = ...
6 years ago (2014-12-10 07:11:46 UTC) #12
tkent
lgtm
6 years ago (2014-12-10 07:14:55 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786753003/100001
6 years ago (2014-12-10 07:16:18 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/37747)
6 years ago (2014-12-10 08:21:45 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786753003/100001
6 years ago (2014-12-10 09:05:20 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/37765)
6 years ago (2014-12-10 10:32:31 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786753003/100001
6 years ago (2014-12-10 12:33:59 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/786753003/120001
6 years ago (2014-12-10 13:15:18 UTC) #26
commit-bot: I haz the power
6 years ago (2014-12-10 14:31:14 UTC) #27
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186848

Powered by Google App Engine
This is Rietveld 408576698