Bug 51379 – Convert <keygen> option elements to a shadow DOM

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

<keygen> needs a shadow DOM to hold its <option> elements.  Right now they're just added to the DOM directly, which causes us to skip the html5 parsing tests on all platforms other than mac (since the current expected DOM is the one which appears for Mac.)

This came up in the review for <https://bugs.webkit.org/show_bug.cgi?id=44907>

[Eric Seidel (no email)]

I'm certain we already have a old bug on this.  :)

[Eric Seidel (no email)]

Hmm.. I guess it was only ever discussed in https://bugs.webkit.org/show_bug.cgi?id=42776.  Maybe a bug about this specific issue was never filed.

[Adam Barth]

Thanks dominicc.  I've been hoping someone would fix this bug.  :)

[Dominic Cooney]

Created attachment 78788 [details]
WIP--adds IDL for keygen element

[Dominic Cooney]

Created attachment 81042 [details]
WIP—moves select into shadow DOM

WIP--moves select into shadow DOM but does not forward form protocol yet

[Dominic Cooney]

Created attachment 81197 [details]
Patch.

Chromium expectations need to be updated.

[Dimitri Glazkov (Google)]

Comment on attachment 81197 [details]
Patch.

This looks great!

[Adam Barth]

So exciting.

[Eric Seidel (no email)]

We shoudl also now remove the parser test from the windows/gtk/qt skipped lists. :)

[Dominic Cooney]

Created attachment 81395 [details]
Unskips html5lib/runner.html for Qt and GTK

[Eric Seidel (no email)]

Comment on attachment 81395 [details]
Unskips html5lib/runner.html for Qt and GTK

LGTM.

[WebKit Commit Bot]

Comment on attachment 81395 [details]
Unskips html5lib/runner.html for Qt and GTK

Rejecting attachment 81395 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7700543

[Eric Seidel (no email)]

Comment on attachment 81395 [details]
Unskips html5lib/runner.html for Qt and GTK

I'm not sure why the cq did that.  Lets try again.

[WebKit Commit Bot]

Comment on attachment 81395 [details]
Unskips html5lib/runner.html for Qt and GTK

Rejecting attachment 81395 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7707156

[Eric Seidel (no email)]

Ah, I see.  That would suggest you're missing a ChangeLog.  But I thought we had a better message for that.

[Dominic Cooney]

My fault. I went from work to home and snarfed the patch with webkit-patch apply. It must have helpfully filled in dglazkov since the previous patch was r+.

[Hajime Morrita]

Committed r77781: <http://trac.webkit.org/changeset/77781>

[Ilya Tikhonovsky]

new expectations were added for chromium

http://trac.webkit.org/changeset/78046
http://trac.webkit.org/changeset/78037

[Eric Seidel (no email)]

Chromium shouldn't need separate expectations anymore.  Do you know why it still has its own -expected.txt file?

[Ilya Tikhonovsky]

(In reply to comment #19)
> Chromium shouldn't need separate expectations anymore.  Do you know why it still has its own -expected.txt file?

There is a difference in SyntaxError text for html5lib/runner.html.

loislo@loislo0-g:/var/sdb1/loislo/chromium/src/third_party/WebKit$ diff LayoutTests/html5lib/runner-expected.txt LayoutTests/platform/chromium/html5lib/runner-expected.txt 
1,2c1,2
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
---
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <
6,9c6,9
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
< CONSOLE MESSAGE: line 1: SyntaxError: Parse error
---
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <
> CONSOLE MESSAGE: line 1: Uncaught SyntaxError: Unexpected token <


and 2 pixel difference of vertical size for fast/invalid/residual-style-expected.txt.

[Adam Barth]

(In reply to comment #19)
> Chromium shouldn't need separate expectations anymore.  Do you know why it still has its own -expected.txt file?

It's because V8's error messages don't match those of JSC.  We've tried getting the V8 team to change, but they refuse.  I'd really like to know how much engineering time that costs us.