WICG / webcomponents

Web Components specifications
Other
4.36k stars 370 forks source link

Closed flag proposal breaks ability to audit and automate tests of web pages #354

Closed dylanb closed 7 years ago

dylanb commented 8 years ago

If web components are created with the closed flag and these web components create interactive elements within their shadow DOM, it is not possible for auditing tools to check for the validity of that markup.

Two examples of auditing that are useful and currently being done

  1. Accessibility auditing
  2. Standards compliance auditing (HTML spec, ARIA spec)

In addition, if a user of that component wishes to use automation tools like Selenium to automate the testing of that component, it is not possible to interact with the shadow DOM components.

Both of these problems present major obstacles to efficient development of quality web applications and therefore represent a failure on the part of this specification to enhance and ease the development of quality, interactive web applications.

There are two potential solutions:

  1. Remove this closed flag altogether
  2. Expose the composed tree through a set of APIs that will allow automation and auditing to occur regardless of the closed/open mode. This should include an API to efficiently discover and return the DOM nodes that have a shadow root and should allow for querySelector* to operate within this composed tree.

If the browser vendors are in favor of 2, I would volunteer to write a proposal

domenic commented 8 years ago

2 seems logically equivalent to 1.

dylanb commented 8 years ago

2 is different from 1 in the following ways:

  1. There is currently no efficient way to determine which nodes have a shadow root,
  2. When you access the shadow root, you are accessing the DOM of the shadow root
  3. The composed tree can replace the entire light DOM tree of the node according to the following algorithm http://w3c.github.io/webcomponents/spec/shadow/#composition-algorithm
  4. This composed tree would not be able to be directly modified
rniwa commented 8 years ago

There is nothing that prevents authors from exposing shadow DOM via JavaScript. e.g. you can add shadowRoot property on the shadow host during the testing.

The whole point of this flag is so that components can enforce the encapsulation as they wish. In the case of debugging and testing, each component should be either:

  1. made by the page author in which case the author can expose the shadow root during testing,
  2. provided by a third party in which case standards compliance, accessibility, and other kinds of testing should be done separately.
dylanb commented 8 years ago

There is nothing that prevents authors from exposing shadow DOM via JavaScript. e.g. you can add shadowRoot property on the shadow host during the testing.

This means that you have different code when testing and when running in production. This is anything but a best practice

The whole point of this flag is so that components can enforce the encapsulation as they wish.

I am struggling to understand why components NEED to enforce this level of encapsulation. Note: encapsulation is enforced by the rest of the spec. This just makes it impossible, instead of difficult, to look inside a component. Why do we need to make it impossible? What features of the web will be impossible to create is we do not have this feature?

In the case of debugging and testing, each component should be either:

made by the page author in which case the author can expose the shadow root during testing, provided by a third party in which case standards compliance, accessibility, and other kinds of testing should be done separately.

Unfortunately this excludes one very necessary type of testing and that is acceptance testing. It is often necessary for people doing acceptance testing (of which accessibility audits are a small piece) to verify whether the deliverable being given to them meets their requirements. It is not acceptable for them to accept the statement of the developer because this does not (in the case of accessibility) in any way free the organization from the legal obligation that it be accessible.

This means that organizations who are not the developers of a component must test its accessibility when they use it and often, organizations who are not the developer of the entire application in which a component is included and where the developer of the application is yet a different third party must validate the entire application.

If we do not allow these organizations access to the most efficient means of testing (both for accessibility and for functionality), then we are making the development of web applications more expensive because we are forcing them to resort to manual testing.

In today's environment, any time we force an organization to resort to manual testing, we are driving up the cost of quality assurance and erecting barriers to quality web applications. The Web Components group of specs (which relies on this spec) is supposed to make development of rich Web-based applications easier, not harder.

The closed flag currently takes us backwards both in the area of automated testing and in the area of accessibility.

rniwa commented 8 years ago

If you'd like to force components to always use the open shadow DOM, you can just do:

var originalAttachShadow = Element.prototype.attachShadow;
Element.prototype.attachShadow = function(options) {
    options['mode'] = 'open';
    return originalAttachShadow(options);
}

at the beginning of the document.

dylanb commented 8 years ago

This will not work reliably for testing through a system like Selenium as it will introduce race conditions.

rniwa commented 8 years ago

That seems like a limitation of a specific browser testing framework. Also, you can quite reliably find elements that are visible and do not have a shadow root as follows:

function isElementVisibleAndWithoutShadowRoot(element) {
    var document.createElement('span');
    span.textContent = 'some text';
    span.style.fontSize = '10px';
    span.style.display = 'inline';
    span.style.position = 'relative';
    // Add more styles to make it even more reliable.
    span.slot = '<some random string>';
    element.appendChild(span); // span will not be distributed to any slot if elements has a shadow root.
    var rect = span.getBoundingClientRect();
    return rect.width || rect.height;
}
dylanb commented 8 years ago

The problem is a limitation with all common end-to-end testing frameworks - you cannot determine the order in which your injected JavaScript will execute because you are testing across process boundaries.

The problem with that code is that you have to execute it on every node in the document - this is a very expensive problem for auditing which can lead to it not being practical to use.

annevk commented 8 years ago

We already discussed this exact issue and decided that it's not worth holding this feature back for. I recommend working the browser teams and in particular those creating developer tools to see how this can be addressed.

dylanb commented 8 years ago

@annevk Perhaps you could summarize the discussion that occurred for those of us that were not there:

For example, I have never seen a justification for the need for this feature (other than a stated desire to have it). I trust that the need must have been quite compelling to have justified overriding these two problems without a resolution.

annevk commented 8 years ago

We've had this discussion along with justification and agreement on the mailing list a long time ago. I recommend looking for "encapsulation".

dylanb commented 8 years ago

@annevk I have read through all the archives that I could find that reference the shadow DOM. Below is my summary of the prior discussion.

tl;dr

  1. No use cases that were accepted by the group were ever presented.
  2. Three serious problems were raised about closed shadow DOM and these were never addressed
  3. The arguments in favor of closed shadow DOM are weak and basically come down to opinion
  4. The single attempt at attempting to determine whether closed shadow DOM is a feature developers would want (a survey) surfaced closed shadow DOM as the single LEAST desired feature
  5. The only serious discussion was about the default value and not about the need for closed shadow DOM at all and how that overrides the three serious issues raised.

In summary, if this issue does not get addressed, then my organization will lodge a formal complaint during CfC

Begin of analysis Start of the discussion, summarizing current status https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0217.html

In this discussion, it is clear that there are arguments on both sides (you can see these by linking to the referenced threads).

The argument in favor at this point is essentially the following:

User of component embeds it in their page User pierces ShadowDOM to achieve some goal Component gets updated and changes the internals User upgrades to new component which breaks functionality User complains and goes back to old version (questionable assertion about what an intelligent user would do) Conclusion: this is bad for the entire Web

A second argument can be summarized as:

Everybody else is doing this, so we must do it too

At this point the argument against the totally opaque encapsulation is that it will prevent certain types of functionality (specifically Google feedback)

Without any further documented discussion, a decision is recorded by Dimitri Glazkov to create a flag allowing closed shadow DOM https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0286.html

Almost immediately, the discussion switches to what the default should be https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0289.html

A third argument is introduced https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0323.html, which can be summarized as “I want it more than closed” and a question asked as to why closed is needed at all. The answer refers back to the post summarized above.

A further argument is introduced which summarizes to “Chrome has something like it that is not exposed, so everyone needs it” https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0681.html

Security as a requirement is explicitly excluded https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0333.html

The concern that closed will make test automation difficult or impossible, is raised https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0335.html

Here the argument is raised that closed is for “encapsulation” and nothing else. There is no argument or evidence presented to why this encapsulation needs to be opaque. The argument claims that the criticism is because the feature is not well understood but then fails to explain it. There is a vague reference to built-in components and security that is not further substantiated. https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0336.html

Here the claim is made that the Google feedback use case is not useful because it states that all components could simply be implemented as open. This means that Google would never be able use third party components in order to achieve their use case. So the argument is basically a “don’t use our feature” argument https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0359.html this argument is immediately refuted here https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0361.html and again here https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0364.html

Question is raised as to why shadowRoot is not a sufficient speed bump and the only argument provided here is an assertion that this is not enough https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0363.html no further evidence or use cases are given despite multiple requests

Statement by Ryosuke that “neither proponent of closed or open has convinced the other which is better” https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0368.html but very explicit restrictions on use cases for closed (testing and feedback) had been raised at this point.

Claim, contrary to prior claim, that there was consensus on the flag https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0377.html

Request for use cases for closed https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0367.html this request is never satisfied

A survey was performed to see whether web developers wanted closed shadow DOM, https://www.w3.org/2014/05/20-webapps-minutes.html response was “very low”. In fact it was lowest of all the features polled.

Document created with the “contentious bits” https://lists.w3.org/Archives/Public/public-webapps/2015JanMar/0542.html all discussion at this point is purely about the default. There is no further mention of the issues with closed at all (testing and feedback)

About this time an additional serious issue was raised with accessibility and closed shadow DOM https://www.w3.org/Bugs/Public/show_bug.cgi?id=27775#c11

None of the issues with closed shadow DOM are addressed and no use cases were ever presented and accepted by the group.

Finally, this issue was marked as closed with reference to the prior discussion. As documented above, the prior discussion does not address these concerns and is not sufficient to dismiss them.

annevk commented 8 years ago

You point to discussion in 2014, but apparently failed to follow the links in e.g., Dimitri's email to https://lists.w3.org/Archives/Public/public-webapps/2013JanMar/thread.html#msg535 where this was discussed in some detail. You're welcome to lodge a formal complaint, although I recommend going for a "Formal Objection" as that's what it's called.

chaals commented 8 years ago

As a chair (i.e. from a position of assumed ignorance), I have been trying to follow the discussion into its deepest ratholes and out again. A few thoughts:

If there clearly isn't Consensus, the chairs probably aren't going to call for it - unless we believe there is a single outlier and we should be moving on, and want to confirm that. So while Anne is right that "formal objection" is the process term, we're aiming not to need the formal process, by reaching a broadly held understanding.

It seems to me that this is indeed a relatively contentious issue - even following the links, I see more or less endlessly continued arguments. I've also sat through some of those in face to face meetings, which are sadly imperfectly minuted (trust me, I did a bunch of that imperfect minuting myself).

It also seems to me that a lot of the contention revolves around pretty fundamental lack of shared understanding. While the question of "open or closed by default" seems unlikely to get unanimity, it seems we really have "somewhat open, somewhat closed, and really very closed" on the table, but many arguments are constructed assuming only 2 of those and different pairs in different cases.

I'm going to do some more archæology, and I hope that people can continue to work towards a shared understanding and a consensus based on goodwill and something that is technically sound. But it seems there is a prima facie case for reopening this issue - which is not to say that it would then be resolved differently - hence my wariness about reopening it "summarily".

dylanb commented 8 years ago

@annevk I assume you mean this link https://wiki.whatwg.org/wiki/Component_Model_Use_Cases which is linked-to from this email https://lists.w3.org/Archives/Public/public-webapps/2011AprJun/1386.html.

That document pre-dates all the issues being raised and is therefore not relevant to the discussion that I am arguing should be reopened.

Also, there is no evidence or arguments presented in that document to support the claims that the encapsulation mechanism that is being claimed is required, is in fact required. This is precisely the unresolved issue.

As pointed out by @rniwa above https://github.com/w3c/webcomponents/issues/354#issuecomment-162383744, the primary use case presented in favor of it can be totally circumvented by a page author if they so wish, meaning that the so-called closedness for purposes of preventing page authors from accessing it, is in fact not achieved. This specific concern was raised in the discussion I reference above and never settled (or never documented as settled). Here is the the key issue in that discussion thread being raised https://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0361.html (why is shadowRoot not a sufficient speed bump)

To summarize: we are introducing a new feature into the web that will break at least three applications that are hugely valuable (testing, auditing and applications like Google Feedback) without ever specifically addressing whether this feature is actually needed to achieve the other benefits that web components brings, or whether, on balance, it would be better to do without this proposed new feature and in full knowledge that the benefits of this feature are actually just a ruse and can be circumvented by the specific users who it is designed to "protect" while still managing to stymie the implementors of useful features like the three mentioned above.

EDIT: Also, I challenge anyone to write a longer sentence than that last sentence :-(

annevk commented 8 years ago

I would like to apologize for my behavior earlier. That was clearly unwarranted. I'll reopen the issue to let someone else make a decision here.

In part the reason we have this feature is because we cannot seem to get agreement on shadow DOM without it. That is, at least vendor has stated that if we don't have this they won't play ball.

I'm personally not swayed that this breaks testing, auditing, and Google Feedback anymore than cross-origin <iframe> does, or isolated components will, when we get there. I don't see why we should not leave it up to the developer to make the choice what kind of semantics they want. If you want to use those kind of tools, and not the browser developer tools, you cannot use this feature. Seems simple. If you have noticed that it becomes harder to upgrade your components when developers poke into them (as folks from Apple and Mozilla have repeatedly indicated has been a problem for them), you use this feature and opt out of testing/auditing in a straightforward manner (perhaps you support a different kind of switch for that).

dylanb commented 8 years ago

@annevk apology accepted!

Actually, this proposal will break the internal auditing tools of at least Mozilla and Chrome.

Currently our aXe extension is the de-facto accessibility auditing tool for Firefox and will break with these changes. We have not started work on web components support, in part because we want to see how the spec shakes out.

Chrome's current implementation relies on traversing the shadowRoot https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/DOMUtils.js#L121. And while I know there is some work ongoing to get this extension into the standard dev tools and not make it a separate extension, it is not clear that that work will circumvent/solve the problem.

I would like to contribute to the solution, here are some proposals:

  1. If there is a publicly available implementation of the closed flag, I could attempt to use @rniwa's proposed circumvention and see how that pans out for the use cases I know of (testing and auditing)
  2. I could take the use cases referenced here https://wiki.whatwg.org/wiki/Component_Model_Use_Cases and try to analyze them from a rigorous problem solution perspective
  3. I could try to formally gather from the group members the specific examples of use cases that have caused trouble and document these so we can analyze/discuss them

thoughts?

annevk commented 8 years ago

@Inscriber, presumably when we implement this in Firefox we'd expose closed shadow trees internally?

Also, someone, e.g., @rniwa or @hayatoito, should take ownership of a decision here. The last time the WG discussed this this was non-negotiable, if it still is someone should take ownership of that and communicate it. There's no reason to keep @dylanb in the dark until implementations start shipping and it's too late.

hayatoito commented 8 years ago

@dylanb, you can use today's Chrome to test a closed shadow tree by enabling "Experimental Web Platform" flag. Could you test the proposed circumvention on that?

dylanb commented 8 years ago

@hayatoito any suggestion on how to enable that flag when calling Chrome through Webdriver?

hayatoito commented 8 years ago

Hmm. I am afraid that I could not help. I do not know how Webdriver works.

For chrome, you can control runtime enabled features from the command line. See https://groups.google.com/a/chromium.org/d/msg/blink-dev/0o1HWlGB7BY/BzkCmrHT6NUJ

I guess Webdriver can pass the command line argument somehow.

travisleithead commented 8 years ago

@thejohnjansen does webdriver provide a way to access an element's shadow dom? Has this been considered by the latest webdriver spec?

dylanb commented 8 years ago

@hayatoito do you know the command line flag for turning on the experimental shadow DOM features?

is it enable-experimental-web-platform-features?

rniwa commented 8 years ago

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --enable-blink-features=ShadowDOMV1

mrmr1993 commented 8 years ago

Chrome intention seems to be to ship regardless of resolution.

mrmr1993 commented 8 years ago

Workaround for selenium webdriver does not work; the workaround gets triggered too late. NodeJS test code:

var your_chrome_path = "/usr/bin/google-chrome-unstable";
var webdriver = require('selenium-webdriver'), chrome = require('selenium-webdriver/chrome');
var driver = new webdriver.Builder().
             forBrowser("chrome").
             setChromeOptions((new chrome.Options())
                              .setChromeBinaryPath(your_chrome_path)
                              .addArguments("enable-blink-features=ShadowDOMV1"))
var driverInstance = driver.build();
driverInstance.get("http://jsbin.com/xabazejuqe");
driverInstance.executeScript(function(){
  var origattachshadow = Element.prototype.attachShadow;
  Element.prototype.attachShadow = function(options){
    options.mode = "open";
    return origattachshadow.apply(this,options);
  }
});

Result: "This is some test text" is still inside a closed shadow DOM.

dylanb commented 8 years ago

@mrmr1993 can you post the jsbin code here it has expired

mrmr1993 commented 8 years ago

I derived it from this JSFiddle, so something like:

<html>
 <head>
  <title>Closed Shadow DOM</title>
  <script language="Javascript" type="text/javascript">
    window.addEventListener("load", function(){
      var shadowRoot = document.getElementById("shadow").attachShadow({mode: "closed"});
      shadowRoot.appendChild(document.createTextNode("This is some test text."));
    }, false);
  </script>
 </head>
 <body>
  <div id="shadow"></div>
 </body>
</html>
mrmr1993 commented 8 years ago

How do people feel about explicitly blacklisting developer induced execution contexts from accessing shadowRoot on closed shadow DOM hosts? (Ie. shadowRoot should behave as for open currently, except when it is closed and requested from one of these contexts.)

By developer induced execution contexts, I mean specifically:

By definition, these wouldn't include any user-agent injected code, so any DOM access from

will explicitly be granted shadowRoot permissions on closed shadow DOMs by the spec.

I feel this would be massively preferable to having to modify the spec for every instance of each of these, especially since we can avoid the proliferation of different APIs for accessing closed shadow DOMs that would result. And I believe it would resolve this issue in its entirety.

Would anybody have objections to this being formalised and included in the spec?

domenic commented 8 years ago

"developer induced execution contexts" are the only things that specs deal with. Other tools are allowed to do whatever they want; specs don't constraint them.

mrmr1993 commented 8 years ago

"developer induced execution contexts" are the only things that specs deal with.

I can't seem to find any mention of that in the DOM spec. As far as I can see, the DOM specifications simply describe an API. As such, it seems like these specifications should (ideally) guide any implementations of the DOM API.

Other tools are allowed to do whatever they want; specs don't constraint them.

Technically nobody is constrained by the spec, except by choice (not necessarily their own). If this is (closer to) how this API is intended to be implemented, it seems like this spec is the place for those details.

annevk commented 8 years ago

The specification should only deal in what is observable from the web. What is offered beyond is a competitive place up for grabs by implementers.

dylanb commented 8 years ago

@annevk yes but we could propose features that are dependent on some other specification change being made - for example https://github.com/w3c/webdriver

annevk commented 8 years ago

@dylanb it would help if you were more explicit. Do you mean that you want an extension to WebDriver for bypassing the closed restriction? If so, that seems reasonable and is probably best discussed with the WebDriver community directly.

mrmr1993 commented 8 years ago

Would a non-normative note alongside the open/closed flag be acceptable then? Something describing that the restriction "is only intended to affect code included by the page developer (and variously code included by that code)" and that it is "not intended to break user-agent injected code that depends on DOM navigability", which may be achieved by "treating this state as 'open' for such injected code" would work for me.

annevk commented 8 years ago

I don't see why we'd have that. We don't do that anywhere else either. Extensions can always go beyond the restrictions of standards.

dylanb commented 8 years ago

@mrmr1993 @annevk this proposal breaks stuff on the web that people rely on today. For example, with the current proposal, it would not be possible to find the actual element behind a point with elementFromPoint anymore. This is in addition to the things already mentioned.

So I don't think its simply enough to punt this to another team. If the solution requires a change in another spec, then we should make this spec dependent on that implementation.

hayatoito commented 7 years ago

Let me close this issue since there is no concrete proposal.

mrmr1993 commented 7 years ago

@hayatoito request to revive? Google web fundamentals advice notes the feature as 'should avoid', stating much the same arguments we've been making all along.

To clarify, concrete proposal is to depreciate the mode key of the first argument of attachShadow, making attachShadow({"mode": "closed"}); fall back to the current behaviour of attachShadow({"mode": "open"});.

I'll also note that there is still no automation, accessibility or extension spec/implementation (that I can find) with a workaround for inspecting closed shadow DOMs. I still personally consider these the bare minimum interfaces that should have this privileged(/not suffer diminished?) access.

hayatoito commented 7 years ago

Thank you for the proposal. However, your proposal is not new one. AFAIR, Google is always proposing the same one, however, the current one is what we agreed on. Reviving this issue might be okay, but I'm afraid that we would repeat the same discussion again and again here without good amount of feedback from web developers in wild.

Does someone have a good idea how we can proceed further? I am not 100% sure whether re-opening this issue is really a good idea or not.

mrmr1993 commented 7 years ago

@hayatoito For better or for worse, I've opened #640, which tries to generalise the idea here. Hopefully the inclusion of the following might prompt some different discussion:

Closed shadow DOMs may (or may not) be fine for document authors, who make the decision to use code including them. However, their existence and use makes the DOM an unreliable API for programs, libraries, etc. that are made to act upon a document by the user or document author.

If the DOM is supposed to be an effective API for developers other than document/component authors, then closed shadow DOMs are directly harmful. ...