|
|
Created:
4 years, 7 months ago by pals Modified:
4 years, 6 months ago Reviewers:
tkent CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake menuitem non-void element.
Allow menuitem to have endtag and menuitem element's end tag can be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or if there is no more content in the parent element.
Specifications:
http://www.whatwg.org/specs/web-apps/current-work/#the-menuitem-element
BUG=87553
Patch Set 1 #Patch Set 2 : NeedsManualRebaseline #Patch Set 3 : Rebased #
Messages
Total messages: 37 (14 generated)
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955863002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955863002/1
Description was changed from ========== Make menuitem non-void element. Specifications: http://www.whatwg.org/specs/web-apps/current-work/#the-menuitem-element BUG=87553 ========== to ========== Make menuitem non-void element. Allow menuitem to have endtag and menuitem element's end tag can be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or if there is no more content in the parent element. Specifications: http://www.whatwg.org/specs/web-apps/current-work/#the-menuitem-element BUG=87553 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955863002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
imported/web-platform-tests/html/syntax/parsing/html5lib_tests25.html expected: #document\n| <!DOCTYPE html>\n| <html>\n| <head>\n| <body>\n| <menuitem>\n| \"A\" actual : #document\n| <!DOCTYPE html>\n| <html>\n| <head>\n| <body>\n| <menuitem>\n| \"A\"
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955863002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
sanjoy.pal@samsung.com changed reviewers: + tkent@chromium.org
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955863002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955863002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take a look.
imported/web-platform-tests/html/syntax/parsing/html5lib_tests25.html expected: #document\n| <!DOCTYPE html>\n| <html>\n| <head>\n| <body>\n| <menuitem>\n| \"A\" actual : #document\n| <!DOCTYPE html>\n| <html>\n| <head>\n| <body>\n| <menuitem>\n| \"A\" above test case is failing. Looks like indentation problem of \"A"\.
Do you know this change won't fix crbug.com/412945, and won't unblock shipping the feature?
On 2016/05/11 00:47:50, tkent wrote: > Do you know this change won't fix crbug.com/412945, and won't unblock shipping > the feature? Ya. I checked that this change is not fixing the apple site issue. Few developers in blink-dev are supportive about shipping with the current state. Can we ship this feature, WDYT?
I think we can ship the feature if - Fix crbug.com/412945, or - Prove that the number of broken site is ignorable This CL and crbug.com/412945 mean the current specification is compatible with neither the web nor Firefox. According to https://www.chromestatus.com/metrics/feature/timeline/popularity/1104, at most 0.3% of pages will be broken by the feature. This number is not acceptable. We need to introduce a new UseCounter to collect more accurate number.
On 2016/05/12 03:29:57, tkent wrote: > I think we can ship the feature if > - Fix crbug.com/412945, or > - Prove that the number of broken site is ignorable > > This CL and crbug.com/412945 mean the current specification is compatible with > neither the web nor Firefox. > According to > https://www.chromestatus.com/metrics/feature/timeline/popularity/1104, at most > 0.3% of pages will be broken by the feature. This number is not acceptable. We > need to introduce a new UseCounter to collect more accurate number. The current specification is fixing crbug.com/412945. The specification says "Allow menuitem to have endtag and menuitem element's end tag can be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or if there is no more content in the parent element." In the patch set:3, I was not checking if immediate next element is menuitem, hr, or menu element. I have put a check (hack :)) for this condition in patch set:4 which solves this apple site issue.
> The current specification is fixing crbug.com/412945. The specification says "Allow menuitem to have endtag and menuitem element's end tag can be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or if there is no more content in the parent element." In the patch set:3, I was not checking if immediate next element is menuitem, hr, or menu element. I have put a check (hack :)) for this condition in patch set:4 which solves this apple site issue. Patch Set 3 correctly implemented the specification. The specification says > A menuitem element's end tag may be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or ... It means "<menuitem>foo</menuitem><menu>bar</menu>" and "<menuitem>foo<menu>bar</menu>" are equivalent. It implies a menuitem element can't have menuitem/hr/menu children.
> The current specification is fixing crbug.com/412945. The specification says "Allow menuitem to have endtag and menuitem element's end tag can be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or if there is no more content in the parent element." In the patch set:3, I was not checking if immediate next element is menuitem, hr, or menu element. I have put a check (hack :)) for this condition in patch set:4 which solves this apple site issue. Patch Set 3 correctly implemented the specification. The specification says > A menuitem element's end tag may be omitted if the menuitem element is immediately followed by a menuitem, hr, or menu element, or ... It means "<menuitem>foo</menuitem><menu>bar</menu>" and "<menuitem>foo<menu>bar</menu>" are equivalent. It implies a menuitem element can't have menuitem/hr/menu children.
On 2016/05/12 07:59:41, tkent wrote: > > The current specification is fixing crbug.com/412945. The specification says > "Allow menuitem to have endtag and menuitem element's end tag can be omitted if > the menuitem element is immediately followed by a menuitem, hr, or menu element, > or if there is no more content in the parent element." In the patch set:3, I was > not checking if immediate next element is menuitem, hr, or menu element. I have > put a check (hack :)) for this condition in patch set:4 which solves this apple > site issue. > > Patch Set 3 correctly implemented the specification. > > The specification says > > A menuitem element's end tag may be omitted if the menuitem element is > immediately followed by a menuitem, hr, or menu element, or ... > > It means "<menuitem>foo</menuitem><menu>bar</menu>" and > "<menuitem>foo<menu>bar</menu>" are equivalent. It implies a menuitem element > can't have menuitem/hr/menu children. <menuitem><span>foo</span><menu>bar</menu></menuitem> In this case I think it should not be <menuitem><span>foo</span></menuitem><menu>bar</menu> as <menuitem> is not immediately followed by <menu>. I shall ask for clarification on https://github.com/whatwg/html/pull/907.
On 2016/05/12 08:50:27, pals wrote: > On 2016/05/12 07:59:41, tkent wrote: > > > The current specification is fixing crbug.com/412945. The specification says > > "Allow menuitem to have endtag and menuitem element's end tag can be omitted > if > > the menuitem element is immediately followed by a menuitem, hr, or menu > element, > > or if there is no more content in the parent element." In the patch set:3, I > was > > not checking if immediate next element is menuitem, hr, or menu element. I > have > > put a check (hack :)) for this condition in patch set:4 which solves this > apple > > site issue. > > > > Patch Set 3 correctly implemented the specification. > > > > The specification says > > > A menuitem element's end tag may be omitted if the menuitem element is > > immediately followed by a menuitem, hr, or menu element, or ... > > > > It means "<menuitem>foo</menuitem><menu>bar</menu>" and > > "<menuitem>foo<menu>bar</menu>" are equivalent. It implies a menuitem element > > can't have menuitem/hr/menu children. > > <menuitem><span>foo</span><menu>bar</menu></menuitem> > > In this case I think it should not be > > <menuitem><span>foo</span></menuitem><menu>bar</menu> > > as <menuitem> is not immediately followed by <menu>. > > I shall ask for clarification on https://github.com/whatwg/html/pull/907. As per patch set 3, <menuitem><span>foo</span><menu>bar</menu></menuitem> becomes <menuitem><span>foo</span></menuitem><menu>bar</menu>
Patchset #4 (id:80001) has been deleted
Checked with the spec authors. As you said, menuitem cannot have children. So, this spec change does not fix the apple site issue. As this patch set make menuitem more spec compliant, can you please review patch set 3?
On 2016/06/15 at 06:38:12, sanjoy.pal wrote: > Checked with the spec authors. As you said, menuitem cannot have children. So, this spec change does not fix the apple site issue. > As this patch set make menuitem more spec compliant, can you please review patch set 3? You should work on changing the specification so that it is compatible with Firefox and the Apple site. Making Blink spec-compliant now helps nothing. |