In some case, we need to dump shadow tree as a part of markup dump.
Created attachment 99669 [details] Patch
Ryosuke, Dimitri, could you take a look?
Comment on attachment 99669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99669&action=review > LayoutTests/ChangeLog:6 > + Added Markup.showShadows() to enable shadow-related dump. Long description goes after "Reviewed by" > LayoutTests/resources/dump-as-markup.js:214 > + var pseudoId = internals.shadowPseudoId(node); > + if (pseudoId) You should check the existence of internals so that opening the test manually doesn't throw errors.
Created attachment 99676 [details] Patch
Comment on attachment 99669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99669&action=review HI Ryosuke, thank you for reviewing quickly! I updated the patch to address the points. >> LayoutTests/ChangeLog:6 >> + Added Markup.showShadows() to enable shadow-related dump. > > Long description goes after "Reviewed by" Oops. Fixed. >> LayoutTests/resources/dump-as-markup.js:214 >> + if (pseudoId) > > You should check the existence of internals so that opening the test manually doesn't throw errors. Fixed.
Comment on attachment 99676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99676&action=review > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup.html:1 > +<html> DOCTYPE? > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup.html:1 > +<html> Ditto about DOCTYPE. > LayoutTests/resources/dump-as-markup.js:224 > + str += "<@shadowRoot>"; I'm not convinced that prepending @ is a good syntax here. Maybe use shadow as a namespace as in <shadow:root shadow:pseudoId=id>?
Created attachment 99683 [details] Patch
Thanks for the second round... (In reply to comment #6) > (From update of attachment 99676 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99676&action=review > > > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup.html:1 > > +<html> > > DOCTYPE? Done. > > > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup.html:1 > > +<html> > > Ditto about DOCTYPE. Done. > > > LayoutTests/resources/dump-as-markup.js:224 > > + str += "<@shadowRoot>"; > > I'm not convinced that prepending @ is a good syntax here. Maybe use shadow as a namespace as in <shadow:root shadow:pseudoId=id>? I have no good idea here.... Okay, let's adopt the namespace style.
Comment on attachment 99683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99683&action=review Should we just always dump shadows? How many existing tests would be affected by doing that? > LayoutTests/resources/dump-as-markup.js:129 > +Markup.showShadows = function() > +{ > + Markup._showShadows = true; Bikeshed nit: I find showShadows a bit confused. show implies to me that it's messing with visibility or display state. Maybe call this dumpShadows?
Created attachment 99776 [details] Patch
> > LayoutTests/resources/dump-as-markup.js:129 > > +Markup.showShadows = function() > > +{ > > + Markup._showShadows = true; > > Bikeshed nit: I find showShadows a bit confused. show implies to me that it's messing with visibility or display state. Maybe call this dumpShadows? Done. I have a opinion here, but I'd just pick my bike today ;-o
Comment on attachment 99776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > LayoutTests/resources/dump-as-markup.js:215 > + str += Markup._indent(depth + 1) + 'shadow:shadowPseudoId="' + pseudoId + '"'; Can this be shadow:pseudoId and shadow:root as Ryosuke suggested? Repeating "shadow" seems unnecessary.
Created attachment 99926 [details] Patch
(In reply to comment #12) > (From update of attachment 99776 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. Yes for the backward compatibility - almost all <input> element have shadows, So enabling shadow dump by default will break existing expectation.. > > > LayoutTests/resources/dump-as-markup.js:215 > > + str += Markup._indent(depth + 1) + 'shadow:shadowPseudoId="' + pseudoId + '"'; > > Can this be shadow:pseudoId and shadow:root as Ryosuke suggested? Repeating "shadow" seems unnecessary. Totally makes sense. Renamed.
(In reply to comment #14) > (In reply to comment #12) > > (From update of attachment 99776 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > Yes for the backward compatibility - almost all <input> element have shadows, > So enabling shadow dump by default will break existing expectation.. Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > (From update of attachment 99776 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > > Yes for the backward compatibility - almost all <input> element have shadows, > > So enabling shadow dump by default will break existing expectation.. > > Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js That's what I was thinking.
Created attachment 99952 [details] Patch
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #12) > > > > (From update of attachment 99776 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > > > Yes for the backward compatibility - almost all <input> element have shadows, > > > So enabling shadow dump by default will break existing expectation.. > > > > Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js > > That's what I was thinking. Well, so I made it default and updated expectations. Actually the number of affected test are smaller than I imagined.
Comment on attachment 99952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99952&action=review r=me provided you address the following points. > LayoutTests/ChangeLog:8 > + - Added Markup.showShadows() to enable shadow-related dump. Please update ChangeLog. > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup-expected.txt:1 > + You should explain what output you'd expect to see. > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup-expected.txt:1 > + Ditto.
Created attachment 100063 [details] Asking for cq+
Comment on attachment 100063 [details] Asking for cq+ View in context: https://bugs.webkit.org/attachment.cgi?id=100063&action=review > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup-expected.txt:2 > +Both meter elements should have a nested shadow box with a width specified.: Period before colon? > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup-expected.txt:2 > +A progress element should have a nested shadow box with a width specified.: Ditto.
Created attachment 100064 [details] Patch
Comment on attachment 100064 [details] Patch Clearing flags on attachment: 100064 Committed r90608: <http://trac.webkit.org/changeset/90608>
All reviewed patches have been landed. Closing bug.