Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841473002:
Implement HTMLMenuItemElement.label (Closed)
DescriptionImplement HTMLMenuItemElement.label
This CL implements HTMLMenuItemElement.label so that the behavior matches the current spec.
The label of a menuitem element is the value of the label content attribute, if there is one and its value is not the empty string, or, otherwise, the result of stripping and collapsing ASCII whitespace from the child text content of the menuitem element.
https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label
https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label
BUG=713431
Review-Url: https://codereview.chromium.org/2841473002
Cr-Commit-Position: refs/heads/master@{#467487}
Committed: https://chromium.googlesource.com/chromium/src/+/ae4d8f00071215f9dfe05e1d762f34833c0eec9b
Patch Set 1 #Patch Set 2 : Add layout test description #Patch Set 3 : Fix syntax error #
Total comments: 15
Patch Set 4 : Reflect review comments #Patch Set 5 : Reflect review comments #
Total comments: 10
Patch Set 6 : Reflect review comments #
Messages
Total messages: 34 (25 generated)
Description was changed from ========== Implement HTMLMenuItemElement.label This CL implements HTMLMenuItemElement.label so that the behavior matches the current spec. The label of a menuitem element is the value of the label content attribute, if there is one and its value is not the empty string, or, otherwise, the result of stripping and collapsing ASCII whitespace from the child text content of the menuitem element. https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label BUG= ========== to ========== Implement HTMLMenuItemElement.label This CL implements HTMLMenuItemElement.label so that the behavior matches the current spec. The label of a menuitem element is the value of the label content attribute, if there is one and its value is not the empty string, or, otherwise, the result of stripping and collapsing ASCII whitespace from the child text content of the menuitem element. https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label BUG=713431 ==========
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
yuzus@chromium.org changed reviewers: + tkent@chromium.org
PTAL :) This is how I feel about this patch: http://i.imgur.com/8jqxDoB.gif
https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:63: if (!label.IsNull() && label != "") if (!label.IsEmpty()) is simpler. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:65: return this->textContent(true).StripWhiteSpace(IsHTMLSpace<UChar>) The argument of textContent() means convert_brs_to_newlines, and the specification doesn't say we should convert BRs. So the argument should be |false|.
https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:1: <!DOCTYPE html> This is a conformance test without any Blink-specific part. We should put this into LayoutTests/external/wpt/html/semantics/interactive-elements/the-menuitem-element/. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:8: We need test cases with - leading/trailing whitespaces in textContent - leading/trailing/sequence-of whitespaces in label attribute https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:11: var item1 = document.getElementById('item1'); Variable |item1| isn't so helpful. We can write simply: assert_equals(document.querySelector('#item1').label, 'item1'); https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:61: const String HTMLMenuItemElement::label() const { The return value should not be |const|. It's unnecessary constraint. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:63: if (!label.IsNull() && label != "") On 2017/04/25 at 09:56:34, tkent wrote: > if (!label.IsEmpty()) > > is simpler. I found the specification didn't ask to check an empty attribute value. This should be |if (!label.IsNull())|. https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label > ... if there is a label content attribute, must return that attribute's value; ... So |label| IDL attribute value and |label| concept are different if |label| content attribute is empty. So confusing! Also, we should update CustomContextMenuProvider::AppendMenuItem() so that it refers to label IDL attribute value or label concept instead of label content attribute. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.h:16: const String label() const; We usually write declarations in the following order: constructors or factory functions destructor other boilerplate functions like DECLARE_VIRTUAL_TRACE member functions So, public: DECLARE_NODE_FACTORY(HTMLMenuItemElement); String label() const; void setLabel(const AtomicString&); is preferable.
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review! PTAL again :) This is how I feel about this patch: http://i.imgur.com/OLt9vOq.gif https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:1: <!DOCTYPE html> On 2017/04/25 at 23:24:59, tkent wrote: > This is a conformance test without any Blink-specific part. We should put this into LayoutTests/external/wpt/html/semantics/interactive-elements/the-menuitem-element/. Done. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:8: On 2017/04/25 at 23:24:59, tkent wrote: > We need test cases with > - leading/trailing whitespaces in textContent > - leading/trailing/sequence-of whitespaces in label attribute Done. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/html/menu_menuitem/menuitem-label.html:11: var item1 = document.getElementById('item1'); On 2017/04/25 at 23:24:59, tkent wrote: > Variable |item1| isn't so helpful. We can write simply: > assert_equals(document.querySelector('#item1').label, 'item1'); Done. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:61: const String HTMLMenuItemElement::label() const { On 2017/04/25 at 23:25:00, tkent wrote: > The return value should not be |const|. It's unnecessary constraint. Right! Done. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:63: if (!label.IsNull() && label != "") On 2017/04/25 at 09:56:34, tkent wrote: > if (!label.IsEmpty()) > > is simpler. Changed the code to follow the spec. Thanks. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.cpp:65: return this->textContent(true).StripWhiteSpace(IsHTMLSpace<UChar>) On 2017/04/25 at 09:56:34, tkent wrote: > The argument of textContent() means convert_brs_to_newlines, and the specification doesn't say we should convert BRs. So the argument should be |false|. Right, done. https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/2841473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.h:16: const String label() const; On 2017/04/25 at 23:25:00, tkent wrote: > We usually write declarations in the following order: > > constructors or factory functions > destructor > other boilerplate functions like DECLARE_VIRTUAL_TRACE > member functions > > So, > > public: > DECLARE_NODE_FACTORY(HTMLMenuItemElement); > > String label() const; > void setLabel(const AtomicString&); > > is preferable. Done. Thanks!
https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:2: <script src='../../../../../../resources/testharness.js'></script> Please use absolute link in external/wpt/. <script src='/resources/testharness.js'></script> https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:3: <script src='../../../../../../resources/testharnessreport.js'></script> Ditto. https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.h:18: String label() const; nit: Add a comment of the spec URL. // https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label String label() const; void setLabel(const AtomicString&); // https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label String conceptualLabel() const; https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/CustomContextMenuProvider.cpp:72: String label_string = menu_item->label(); Can you check Firefox behavior for <menuitem label="">text content</menuitem>? If Firefox shows "text content" in the menu, it uses conceptualLabel() here.
Thanks for the review! PTAL again :) This is how I feel about this patch: http://i.imgur.com/3waERlN.gif https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:2: <script src='../../../../../../resources/testharness.js'></script> On 2017/04/26 at 08:59:58, tkent wrote: > Please use absolute link in external/wpt/. > > <script src='/resources/testharness.js'></script> Done. https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:3: <script src='../../../../../../resources/testharnessreport.js'></script> On 2017/04/26 at 08:59:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:3: <script src='../../../../../../resources/testharnessreport.js'></script> On 2017/04/26 at 08:59:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMenuItemElement.h (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMenuItemElement.h:18: String label() const; On 2017/04/26 at 08:59:58, tkent wrote: > nit: Add a comment of the spec URL. > > // https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label > String label() const; > void setLabel(const AtomicString&); > > // https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label > String conceptualLabel() const; Done. https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/CustomContextMenuProvider.cpp (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/CustomContextMenuProvider.cpp:72: String label_string = menu_item->label(); On 2017/04/26 at 08:59:58, tkent wrote: > Can you check Firefox behavior for <menuitem label="">text content</menuitem>? If Firefox shows "text content" in the menu, it uses conceptualLabel() here. Firefox does not render the menuitem when label is empty and text content exists, which means label is set to an empty string. To match the behavior I'll keep it this way here.
The CQ bit was checked by tkent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tkent@chromium.org
lgtm https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html (right): https://codereview.chromium.org/2841473002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/semantics/interactive-elements/the-menu-element/menuitem-label.html:2: <script src='../../../../../../resources/testharness.js'></script> BTW, here is the document about external/wpt/: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat... I'll explain more offline.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493245949748940,
"parent_rev": "bafc23e1920cc799c03159c39e29878e26071db0", "commit_rev":
"ae4d8f00071215f9dfe05e1d762f34833c0eec9b"}
Message was sent while issue was closed.
Description was changed from ========== Implement HTMLMenuItemElement.label This CL implements HTMLMenuItemElement.label so that the behavior matches the current spec. The label of a menuitem element is the value of the label content attribute, if there is one and its value is not the empty string, or, otherwise, the result of stripping and collapsing ASCII whitespace from the child text content of the menuitem element. https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label BUG=713431 ========== to ========== Implement HTMLMenuItemElement.label This CL implements HTMLMenuItemElement.label so that the behavior matches the current spec. The label of a menuitem element is the value of the label content attribute, if there is one and its value is not the empty string, or, otherwise, the result of stripping and collapsing ASCII whitespace from the child text content of the menuitem element. https://html.spec.whatwg.org/multipage/forms.html#dom-menuitem-label https://html.spec.whatwg.org/multipage/forms.html#concept-menuitem-label BUG=713431 Review-Url: https://codereview.chromium.org/2841473002 Cr-Commit-Position: refs/heads/master@{#467487} Committed: https://chromium.googlesource.com/chromium/src/+/ae4d8f00071215f9dfe05e1d762f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ae4d8f00071215f9dfe05e1d762f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
