A lot of test cases require the shadow dom to be enabled: editing/shadow fast/dom/shadow The feature is already enabled by default for GTK port.
Created attachment 145035 [details] Patch
Comment on attachment 145035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145035&action=review > LayoutTests/platform/efl/test_expectations.txt:97 > +BUGWK86623 : fast/dom/shadow/drop-event-for-input-in-shadow.html = TEXT I think it is good to list up by alphabetical sorting.
Created attachment 145291 [details] Patch Attempt to make the GTK bot happy.
Created attachment 145297 [details] Patch Simplify patch thanks to dependency bugs. Does anyone know why it does not build for the GTK port? I added "JSGenerateToJSObject" extended attribute to ShadowRoot.idl so that a toJS() method is generated for JSShadowRoot. This works fine on EFL port but the GTK port gets a linking error for the new toJS() method. I'm assuming some Makefile needs to be tweaked somehow?
I guess code generator for JSC isn't aware of ShadowRoot. We need to teach it about ShadowRoot. (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.) Filed Bug 88211. Haraken, do you have any ideas?
(In reply to comment #5) > I guess code generator for JSC isn't aware of ShadowRoot. > We need to teach it about ShadowRoot. > (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.) > > > Filed Bug 88211. > Haraken, do you have any ideas? Do you mean that the corresponding change to this (http://trac.webkit.org/changeset/108035/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm) would be needed in CodeGeneratorJS.pm to support ShadowRoot? If my understanding is correct, the corresponding change is not needed in CodeGeneratorJS.pm since JS bindings do not distinguish between DOMNode, DOMObject and ActiveDOMObject etc.
(In reply to comment #6) > (In reply to comment #5) > > I guess code generator for JSC isn't aware of ShadowRoot. > > We need to teach it about ShadowRoot. > > (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.) > > > > > > Filed Bug 88211. > > Haraken, do you have any ideas? > > Do you mean that the corresponding change to this (http://trac.webkit.org/changeset/108035/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm) would be needed in CodeGeneratorJS.pm to support ShadowRoot? > > If my understanding is correct, the corresponding change is not needed in CodeGeneratorJS.pm since JS bindings do not distinguish between DOMNode, DOMObject and ActiveDOMObject etc. Good point. The compilation error is telling us that we need toJS(ShadowRoot*) or something like that. But I don't understand the logic behind that.
(In reply to comment #5) > I guess code generator for JSC isn't aware of ShadowRoot. > We need to teach it about ShadowRoot. > (We did it for V8 when we started working on that at trac.webkit.org/changeset/108035.) > > > Filed Bug 88211. > Haraken, do you have any ideas? I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well. The toJS() method for shadow root is generated correctly for EFL port.
> > I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well. > The toJS() method for shadow root is generated correctly for EFL port. Oh really? that's great news! Then we can just add the missing symbol to the symbols.filter file. http://trac.webkit.org/browser/trunk/Source/autotools/symbols.filter We need to figure out the symbols name, though.
Maybe a clean/full GTK build is required for this patch. It wouldn't be the first time.
(In reply to comment #10) > Maybe a clean/full GTK build is required for this patch. It wouldn't be the first time. Ok, I'll make a local build of the GTK port and make sure this is the issue.
(In reply to comment #9) > > > > I'm not sure what you mean. The EFL port is using JSC as well and it compiles just fine after the small IDL fix I made. The shadow root tests are passing as well. > > The toJS() method for shadow root is generated correctly for EFL port. > Oh really? that's great news! > Then we can just add the missing symbol to the symbols.filter file. > http://trac.webkit.org/browser/trunk/Source/autotools/symbols.filter > We need to figure out the symbols name, though. Thanks, this was the problem. I'll reupload a patch that should build on GTK port.
Created attachment 145531 [details] Patch This patch fixes the GTK build by adding the right symbol to Source/autotools/symbols.filter. A clean rebuild may still be required though.
Green GTK bubble, yay :) Now, we just need to land the dependency patch at Bug 88029.
Created attachment 145577 [details] Patch Update test expectations to indicate why remaining Shadow DOM tests are failing on EFL port.
It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ?
(In reply to comment #16) > It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ? Because it is already there: $ cat Source/cmake/WebKitFeatures.cmake | grep SHADOW_DOM WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)
(In reply to comment #17) > (In reply to comment #16) > > It seems to me there is no critical problems now. By the way, why don't you also add "WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF)" for other ports which are using cmake ? > > Because it is already there: > > $ cat Source/cmake/WebKitFeatures.cmake | grep SHADOW_DOM > WEBKIT_OPTION_DEFINE(ENABLE_SHADOW_DOM "Toggle Shadow DOM support" OFF) If so, there is no problem. LGTM.
Created attachment 145973 [details] Patch Rebase on master. Could someone review the patch please?
Created attachment 145984 [details] Patch Unskip an additional Shadow DOM test by enabling focus cycling in DRT.
Comment on attachment 145984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145984&action=review > Tools/DumpRenderTree/efl/DumpRenderTreeView.cpp:192 > +static Eina_Bool onFocusCanCycle(Ewk_View_Smart_Data *sd, Ewk_Focus_Direction direction) Style nit : s/sd/smartData/g. Or, you can omit parameter name until it is used.
Created attachment 146004 [details] Patch Take Gyuyoung's feedback into consideration, thanks.
Comment on attachment 146004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146004&action=review > LayoutTests/fast/dom/shadow/form-in-shadow-expected.txt:10 > +PASS obj.hidden is 'hidden' > +PASS obj.text is 'text' > +PASS obj.checkbox1 is 'on' > +PASS obj.checkbox2 is undefined. > +PASS obj.range is '50' > +PASS obj.textarea is 'textarea' > +PASS successfullyParsed is true > + > +TEST COMPLETE > + Should this result go in the EFL directory? > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:199 > +#if PLATFORM(EFL) > +bool RuntimeEnabledFeatures::isShadowDOMEnabled = true; > +#else > bool RuntimeEnabledFeatures::isShadowDOMEnabled = false; > #endif > +#endif This seems suspicious. Is this really something that should be turned on by default for all embedders? Perhaps add a setting in the EFL API and have the DRT flip it?
Comment on attachment 146004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146004&action=review >> LayoutTests/fast/dom/shadow/form-in-shadow-expected.txt:10 >> + > > Should this result go in the EFL directory? I can move it, find by me. I just find it strange that there is no global expectation for this test. The test does not look platform specific. >> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:199 >> +#endif > > This seems suspicious. Is this really something that should be turned on by default for all embedders? Perhaps add a setting in the EFL API and have the DRT flip it? Sounds like a good idea. I'll do that.
Created attachment 146227 [details] Patch Take Martin Robinson's feedback into consideration.
Comment on attachment 146227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146227&action=review > Source/WebKit/efl/ewk/ewk_settings.cpp:258 > + return FALSE; s/FALSE/false/g > Source/WebKit/efl/ewk/ewk_settings.cpp:268 > +#endif I think you need to add below code. #else return false; #endif. > Source/WebKit/efl/ewk/ewk_settings.h:288 > + * By default, Shadow DOM is disabled. Do you enable shadow dom in OptionEfl.cmake ? > Source/WebKit/efl/ewk/ewk_settings.h:303 > + * By default, Shadow DOM is disabled. ditto.
Comment on attachment 146227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146227&action=review It looks mostly OK except for the missing #else clause Gyuyoung mentioned. I'm not very familiar with the work on Shadow DOM to know whether it makes sense to enable it by default, or even if it should always be enabled. Does it make the most sense to only enable it in DRT by default? >> Source/WebKit/efl/ewk/ewk_settings.h:288 >> + * By default, Shadow DOM is disabled. > > Do you enable shadow dom in OptionEfl.cmake ? He enabled the feature at build-time in OptionsEfl.cmake; that's different from calling ewk_settings_shadow_dom_enable_set(EINA_TRUE) from ewk_main.cpp, for example.
Created attachment 146481 [details] Patch - Fix FALSE -> false - Return false in #else case Gyuyoung, as rakuco said, the feature is enabled at compile time but disabled by default at runtime. For now, I'm enabling it only for DRT, as the chromium port is doing.
(In reply to comment #28) > Created an attachment (id=146481) [details] > Patch > > - Fix FALSE -> false > - Return false in #else case > > Gyuyoung, as rakuco said, the feature is enabled at compile time but disabled by default at runtime. > For now, I'm enabling it only for DRT, as the chromium port is doing. If so, ok.
Comment on attachment 146481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146481&action=review Looks sane to me, but it'd be good for someone else to double-check the change to the IDL file. > Source/WebCore/dom/ShadowRoot.idl:33 > - ConstructorRaisesException > + ConstructorRaisesException, > + JSGenerateToJSObject I don't really feel confident enough to review to particular part of the change.
(In reply to comment #30) > (From update of attachment 146481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146481&action=review > > Looks sane to me, but it'd be good for someone else to double-check the change to the IDL file. > > > Source/WebCore/dom/ShadowRoot.idl:33 > > - ConstructorRaisesException > > + ConstructorRaisesException, > > + JSGenerateToJSObject > > I don't really feel confident enough to review to particular part of the change. haraken: I believe this is something you could help with? Without this extra extended attribute, the ShadowRoot object looks like a DocumentFragment (its parent) on JS side and we cannot access its properties / methods.
(In reply to comment #31) > > > Source/WebCore/dom/ShadowRoot.idl:33 > > > - ConstructorRaisesException > > > + ConstructorRaisesException, > > > + JSGenerateToJSObject > > > > I don't really feel confident enough to review to particular part of the change. > > haraken: I believe this is something you could help with? > > Without this extra extended attribute, the ShadowRoot object looks like a DocumentFragment (its parent) on JS side and we cannot access its properties / methods. IDL change looks OK. Since ShadowRoot inherits DocumentFragment, by default toJS() is not generated for ShadowRoot. Consequently, the ShadowRoot object is wrapped by inherited toJS() of DocumentFragment, and thus the object looks like a DocumentFragment object. JSGenerateToJSObject prevents the situation by generating toJS() for ShadowRoot. Let me mark r+ based on mrobinson's review on other parts.
Comment on attachment 146481 [details] Patch Clearing flags on attachment: 146481 Committed r120144: <http://trac.webkit.org/changeset/120144>
All reviewed patches have been landed. Closing bug.
*** Bug 80086 has been marked as a duplicate of this bug. ***