Bug 149698 – Rewrite HTMLDetailsElement using HTMLSlotElement

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

[Ryosuke Niwa]

Now that the basic infrastructure for shadow DOM is available, rewrite details element using the new API.

[Ryosuke Niwa]

Created attachment 262237 [details]
WIP

[Ryosuke Niwa]

Maybe I should split the removal into a separate patch?

[Antti Koivisto]

Seems fine like this too.

[Ryosuke Niwa]

(In reply to comment #3)
> Seems fine like this too.

Okay. FWIW, I need to land my patch on https://bugs.webkit.org/show_bug.cgi?id=149701 first.

[Antti Koivisto]

Created attachment 262824 [details]
patch

[WebKit Commit Bot]

Attachment 262824 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SlotAssignment.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 262824 [details]
patch

Attachment 262824 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/264687

New failing tests:
fast/html/details-open2.html

[Build Bot]

Created attachment 262825 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Antti Koivisto]

Created attachment 262826 [details]
patch

[WebKit Commit Bot]

Attachment 262826 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SlotAssignment.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 262826 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262826&action=review

r=me

> Source/WebCore/html/HTMLDetailsElement.cpp:76
> +    addShadowRoot(ShadowRoot::create(document, std::make_unique<SlotAssignment>(slotNameFunction)));

As discussed on IRC, calling this virtual function from a constructor is a bit lame, even though this is a leaf class.
I think we should just leave it in create() like you did with HTMLSummaryElement.

> Source/WebCore/html/HTMLDetailsElement.h:49
>      bool m_isOpen;
> +    HTMLSlotElement* m_summarySlot;
> +    HTMLSummaryElement* m_defaultSummary;

Would be nice to initialize all of these explicitly here.

[Antti Koivisto]

https://trac.webkit.org/r190840

[Brent Fulgham]

This change introduced several Windows test failures because the baselines changed in the same way as the Mac port.

I committed a rebaseline under <https://trac.webkit.org/r190861>.

[Brent Fulgham]

There are a number of new test failures on EFL/GTK and Windows after this change was committed. See Bug 150036 for details.

New failing tests are:

fast/html/details-add-details-child-2.html
fast/html/details-add-summary-child-1.html
fast/html/details-add-summary-child-2.html

[Radar WebKit Bug Importer]

<rdar://problem/23289343>