I would just need to make sure each test suite passes with 1) Shadow DOM enabled && UA Widget enabled 2) Shadow DOM enabled && UA Widget disabled Suites fall into (2) like Reftest would need follow-up filed like bug 1483657.

I've fixed most of the errors, I think. Those are the real errors. I couldn't fix the innerText wpt test here yet, though: https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail.com&selectedJob=194570449 This test can be reduced to this URL data:text/html,<audio style='display:block'><source id='target' class='poke' style='display:block'><script>t = document.getElementById("target"); t.textContent = "abc"; alert(t.innerText);</script> and wpt expects t.innerText to be empty while with UA Widget it returns "abc". (contradicts to intuition, wpt expects innerText of <source> with display:none to return "abc"). So far I've traced here: https://searchfox.org/mozilla-central/rev/ef8b3886cb173d5534b954b6fb7eb2d94a9473d0/dom/html/nsGenericHTMLElement.cpp#2961 With UA Widget, |IsOrHasAncestorWithDisplayNone()| returns true regardless because aElement->HasServoData() evaluates to false. With XBL impl it goes into Servo_Element_IsDisplayNone(aElement) and correctly returns false. Something funny must be happening in layout because I flipped the leaf/non-leaf bit of nsVideoFrame... I would need to look into it. I will submit the first two patches about WebVTT and the pref flip for review first.

Backed out changeset fd0d6079d0d2

The UA Widget videocontrols is contained in a <div class="videocontrols"> instead of an <xul:videocontrols>. The former is by default visible in the accessibility tree unless its role is set to none. Depends on D3665

With UA Widget, the videocontrols container is created lazily. It won't be a problem for WebVTT.processCues() in vtt.jsm, so TextTrackManager::UpdateCueDisplay() should not early return there, but pass nullptr to it. Depends on D3666

Try of current changesets https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8bc1f4d2a71a4af6990ce5fa7c8d8a7588d4fb7

Comment on attachment 9002128 [details] Bug 1484048 - Allow TextTrackManager to pass cue without videocontrols r=alwu Alastor Wu [:alwu] has approved the revision.

Comment on attachment 9002125 [details] Bug 1484048 - Part V, Re-enable UA Widget on Desktop Nightly Olli Pettay [:smaug] has approved the revision.

Comment on attachment 9002127 [details] Bug 1484048 - Part I, Restore the video controls accessible tree :Gijs (he/him) has approved the revision.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c68580481e8a47bc4467f82ddd7f7ed6eb1538f2 accessibility tests still fail intermittently on Windows.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1) > With UA Widget, |IsOrHasAncestorWithDisplayNone()| returns true regardless > because aElement->HasServoData() evaluates to false. This is expected if the comment here holds true: https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/dom/base/Element.h#1985-1986 > Something funny must be happening in layout because I flipped the > leaf/non-leaf bit of nsVideoFrame... I would need to look into it.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #10) > > Something funny must be happening in layout because I flipped the > > leaf/non-leaf bit of nsVideoFrame... I would need to look into it. It's not about the leaf or non-leaf-ness of nsVideoFrame. Is about the flattened tree bit. Children of a Shadow Host are outside of the flattened tree, so there's no element to inherit from. Do other engines pass this test?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11) > Do other engines pass this test? Chrome failed on this test too https://github.com/chromium/chromium/blob/master/third_party/WebKit/LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/getter-expected.txt#L83 So according to the spec in https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute the innerText getter must return the same content as textContent when the node is not "being rendered". I wonder if <source style="display: block"> is considered "being rendered" here. It didn't show up on the screen.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #11) > > Do other engines pass this test? > > Chrome failed on this test too > > https://github.com/chromium/chromium/blob/master/third_party/WebKit/ > LayoutTests/external/wpt/html/dom/elements/the-innertext-idl-attribute/ > getter-expected.txt#L83 > > So according to the spec in > https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute > the innerText getter must return the same content as textContent when the > node is not "being rendered". I wonder if <source style="display: block"> is > considered "being rendered" here. It didn't show up on the screen. That <source> is definitely not being rendered. I'd file a spec bug, and maybe point out at the test.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13) > That <source> is definitely not being rendered. I'd file a spec bug, and > maybe point out at the test. Bug filed on wpt https://github.com/web-platform-tests/wpt/issues/12580

This is a regression left over by Bug 1431255 Part IV. I did s/anonid/id/ in videocontrols.js but I didn't do that in CSS. Depends on D3667

Appearently with UA Widget the page loads quicker, so the tests must now explicitly wait for the audio source to load to test on the video controls UI in its stable state. Depends on D3840

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #13) > > That <source> is definitely not being rendered. I'd file a spec bug, and > > maybe point out at the test. > > Bug filed on wpt https://github.com/web-platform-tests/wpt/issues/12580 Filed bug 1484855 to track this here, and https://github.com/whatwg/html/issues/3947 about display: contents and innerText.

Flipping the UA Widget pref changed the behavior of Gecko such that it now returned contents on these two tests. Per the DOM spec, Gecko should return the content for element not being rendered. The tests therefore should be updated accordingly. https://github.com/web-platform-tests/wpt/issues/12580 was filed on WPT about this. Bug 1484855 will correct the Gecko implementation. Depends on D3841

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fbc64898a42b34a3af6960effdaa9934459e0f3

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16) > Created attachment 9002610 [details] > Bug 1484048 - Part V, Wait for audio source to load before starting a11y > tests r=surkov > > Appearently with UA Widget the page loads quicker, so the tests must now > explicitly wait for the audio source to load to test on the video controls > UI in its stable state. the approach works fine in case of action/test_media.html, but I would tweak it a bit for tree/test_media.html testcase. If @src attribute changes the accessible tree, then it makes sense to listen accessibility events to make sure that proper accessibility events are fired; otherwise screen readers won't reflect the control properly. So could you try to use waitForEventPromise(EVENT_REORDER, mediaControlElement) instead (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/events.js#119)?

Comment on attachment 9002618 [details] (Closed) Bug 1484048 - Part VI, Allow innerText to return content of <source> per spec in wpt tests r=emilio Emilio Cobos Álvarez (:emilio) has approved the revision.

(In reply to alexander :surkov (:asurkov) from comment #20) > the approach works fine in case of action/test_media.html, but I would tweak > it a bit for tree/test_media.html testcase. If @src attribute changes the > accessible tree, then it makes sense to listen accessibility events to make > sure that proper accessibility events are fired; otherwise screen readers > won't reflect the control properly. So could you try to use > waitForEventPromise(EVENT_REORDER, mediaControlElement) instead > (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/ > events.js#119)? I've tried the following async function loadAudioSource() { let el = document.getElementById("audio"); let videoControlsEl = SpecialPowers.unwrap(el).openOrClosedShadowRoot.firstChild; let p = waitForEventPromise(EVENT_REORDER, videoControlsEl); el.src = "../bug461281.ogg"; await p; doTest(); } but it fails with 1:38.04 FAIL Can't get accessible for [ 'div node', address: [object HTMLDivElement] ] getAccessible@chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:301:9 observe@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:129:33 By looking at |event.accessible.DOMNode.tagName| in the observer I can see the tagName was indeed <div>. Do you mind if we move this to a follow-up? We don't test for accessibility events for the XBL version of the video controls either.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24) > By looking at |event.accessible.DOMNode.tagName| in the observer I can see > the tagName was indeed <div>. Do you mind if we move this to a follow-up? We > don't test for accessibility events for the XBL version of the video > controls either. I think I know what's wrong here. The current case fails intermittently because of the following: 1. When the control is initialized on <audio src controls>, it set the error message to be visible in setupInitialState() because the source is present but is not loaded. https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/toolkit/content/widgets/videocontrols.js#240 2. As soon as the loadstart event happens, we hide the error message. https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/toolkit/content/widgets/videocontrols.js#494 Now, if the accessibility test starts between 1 and 2, the assertion will fail because it sees the error message. With my change which loads audio source dynamically, the following happens 1. When the control is initialized on <audio controls>, it shows no error message because there isn't a source. 2. When the src is set, the controls **updates**, without error message showing up in between. That's why I never see an EVENT_REORDER event here. I do however see the progress bar is updated visually and with an EVENT_VALUE_CHANGE event. I still think the patch should be landed as-is. We can create another set of tests like test_media_error.html to assert the media error messages in the accessibility tree.

BTW the <div> that has its children list changes was the one in DevTools :-/ sorry about the confusion.

Comment on attachment 9002610 [details] Bug 1484048 - Part III, Wait for audio source to load before starting a11y tests per comment #25 agreed, we'd rather should have one more test, than modifying this one to handle the error message case. please add a comment in the test why <video src controls> doesn't work.

Comment on attachment 9002610 [details] Bug 1484048 - Part III, Wait for audio source to load before starting a11y tests alexander :surkov (:asurkov) has been removed from the revision.

Another wpt test failure https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail.com&selectedJob=195331491

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #29) > Another wpt test failure > > https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail. > com&selectedJob=195331491 This is more complex than I would like it to be, seems to be a combined effect of document.write() and UA Widget. This test calls document.write() on the media document of the iframe at the time the onload iframe is fired on the iframe. With UA Widget, this caused the onload event to be dispatched twice. I've narrow down that to be caused by having DOM element injected from videocontrols.js; commenting that out will make the problem disappear. Removing document.write() call will also fix the problem.

Comment on attachment 9002607 [details] Bug 1484048 - Part II, Make error label visible :Gijs (he/him) has approved the revision.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30) > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment > #29) > > Another wpt test failure > > > > https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail. > > com&selectedJob=195331491 > > This is more complex than I would like it to be, seems to be a combined > effect of document.write() and UA Widget. > > This test calls document.write() on the media document of the iframe at the > time the onload iframe is fired on the iframe. With UA Widget, this caused > the onload event to be dispatched twice. > > I've narrow down that to be caused by having DOM element injected from > videocontrols.js; commenting that out will make the problem disappear. > Removing document.write() call will also fix the problem. With lldb I can see the second load event is triggered by the document.close() call. This is related to bug 1444872 comment 16 and https://github.com/web-platform-tests/wpt/pull/10209#issuecomment-376914352 . Strangely, I cannot reproduce the test timeout locally as mentioned, so here is Try push that try to reproduce it there. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4b0d78855455764d1b7f56eedf38f847d6bbfb6 Here is a try push to all the patches, if they work. I don't know how to push it to Phabricator without assigning a reviewer (since it's not a proven solution ready for review). https://treeherder.mozilla.org/#/jobs?repo=try&revision=647e390bea80a02b4e942736a1791aecb89ccdee Regardless if a document.close() call inside the load event handler triggers an assertion + another load event, it would be a bug needs to be handled. It's just that I don't think I would be able to handle it here.

Correction: this is Try result with the patch removes document.close() alone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a20101458c3bdd27d455795f0702381ba505bd08 This is the Try result with all the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=323436c967cad417f9ce8e999b5b86e4807378d2

To unblock I would like to workaround the previously mentioned bug for now. Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a68f6e0978bc4d021f73a62b5e18d03a256cbf76

The previous try push only tests the workaround patch. This tests the entire patch set. Let's see if it's good. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d438d879aa4171917925cacff8394f39ff72222

(In reply to alexander :surkov (:asurkov) from comment #28) > Comment on attachment 9002610 [details] > Bug 1484048 - Part V, Wait for audio source to load before starting a11y > tests r=surkov > > alexander :surkov (:asurkov) has been removed from the revision. Did you do this unintentionally? Would you mind set it again in Phabricator? Thanks. https://phabricator.services.mozilla.com/D3841

Start testing on when the parent frame is loaded, workarounds quirks as described in bug 1444872 comment 16. Depends on D3841

Comment on attachment 9002610 [details] Bug 1484048 - Part III, Wait for audio source to load before starting a11y tests alexander :surkov (:asurkov) has approved the revision.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #36) > (In reply to alexander :surkov (:asurkov) from comment #28) > > Comment on attachment 9002610 [details] > > Bug 1484048 - Part V, Wait for audio source to load before starting a11y > > tests r=surkov > > > > alexander :surkov (:asurkov) has been removed from the revision. > > Did you do this unintentionally? Would you mind set it again in Phabricator? > Thanks. > > https://phabricator.services.mozilla.com/D3841 no, just a glitch I think, should be fixed now.

This is what I have found about the test failure for now. :bz, have I encounter an existing bug that looks obvious to you? If not I will keep investigating. Let me know where I should look too.

So I continue to spend time today digging into why onload was called twice. The note is kind of written for my own reference so don't bother reading the whole thing. What I find was, between the test testing PNG media document and Video document, we breached here, in nsDocument::BlockOnload() https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/dom/base/nsDocument.cpp#8180 i.e. at the onload is called on PNG media document, there isn't a script a script blocker, while for video document, there is. This may be because for PNG `document.write()` only clear an image, while for Video it had to clear all the DOM of video and video controls. That along perhaps shouldn't cause a problem, as the branch there adds an AsyncBlockOnload script runner. The function did run, but when it calls into BlockOnload() again, GetDocumentLoadGroup() returns nullptr so we don't really call into loadGroup->AddRequest(mOnloadBlocker) below. https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/dom/base/nsDocument.cpp#8190 Is this expected somehow? Shouldn't mOnloadBlockCount and mAsyncOnloadBlockCount be consistent with the actual state? Let me know if I find the right cause...

Comment on attachment 9002618 [details] (Closed) Bug 1484048 - Part VI, Allow innerText to return content of <source> per spec in wpt tests r=emilio Emilio Cobos Álvarez (:emilio) has been removed from the revision.

I've found a cause and two potential solutions. This happens because in nsIDocument::ResetToURI() it resets and sets mDocumentLoadGroup while the video element is being removed. The first solution is to check the new LoadGroup instance given and see if it's the same weak reference. https://phabricator.services.mozilla.com/D4484 https://treeherder.mozilla.org/#/jobs?repo=try&revision=56036b6d9e16aacbde6a0a0cf0e113686516d5c4 The second simply move that line right above |if (aLoadGroup) {|, so we are not doing anything in between. https://treeherder.mozilla.org/#/jobs?repo=try&revision=21713facf02d479d07711cf637ed166e50028c64 Will ask for review if either is proven to fix the bug on Try.

Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b23d38afcfdd7a8325e98068ab5f8b0eb6df8998

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #37) > Created attachment 9004677 [details] > Bug 1484048 - Part VI, Don't reset mDocumentLoadGroup if the same LoadGroup > is passed Boris, would you be able to review this patch this week? Thanks.

I am aiming to get to it today. Still sorting through the details of what's going on here.

Sorry for the lag here; I finally managed to get enough work time in a row to understand what's going on. Thank you for the logs; they were extremely helpful! So the sequence of events here is: 1) Document::Open() is called. 2) This calls nsIDocument::ResetToURI. 3) This nulls out mDocumentLoadGroup, then removes all the document's kids, then sets the new loadgroup. Removing the kids removes a <video> element, which has attached to it a shadow tree containing <link>. When we remove the element from the DOM, that <link> clears out its old stylesheet and starts a new load. That load is satisfied immediately from the css::Loader parsed-sheet hashtable, so we just queue a task to fire a load event on the <link>. Such tasks need to block onload, so we try to add a load blocker. We're under a scriptblocker, so nsDocument::BlockOnload just queues a scriptrunner to do the real work. But that scriptrunner runs before we set the new loadgroup, so nsDocument::BlockOnload ends up incrementing mOnloadBlockCount but doesn't actually add a load-blocker request to the (nonexistent at that point) loadgroup. After that our state is all messed up, because we have a nonzero mOnloadBlockCount so will ignore future BlockOnload calls. We could conceivably try to put a scriptblocker around more of ResetToURI, so that the new loadgroup value would be set by the time the scriptrunner runs. This has some risk around changing the behavior of the element removals, I guess, sort of. We could examine the value of mOnloadBlockCount before/after the element removals and once we set the new loadgroup value adjust the mOnloadBlockCount to the old value then do however many calls to BlockOnload we're supposed to have now. This would arguably be the safest change, but a bit ugly. We could do what the attached patch is aiming to do, but it means that loads started during the element removals will now end up in the loadgroup. Ideally there would be no such loads, of course.... I did check when that "mDocumentLoadGroup = nullptr;" bit got added, and it's super-old. Like original rpotts code from 1999 old... So it's not necessarily clear why that bit is there at all. Anyway, I think for now the safest thing to do is the attached patch, and then once we do bug 1489308 we can change this code to assert there is no loadgroup on entry, maybe. Would need to check. Could you file a followup bug, depending on bug 1489308, to do that?

Also, I kinda wonder whether we could avoid that load the sheet when the <video> is removed bit...

Comment on attachment 9004677 [details] Bug 1484048 - Part IV, Don't reset mDocumentLoadGroup if the same LoadGroup is passed Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Actually just did that in bug 1490791. There was a TODO I left to optimize out the load, but the CSSWG decided to tie it to document connectedness so that complexity can go away.

I'll land this after bug 1490793 gets to central. I've shuffled the patch a bit so I can land them from Lando one by one. This is the Try without the last patch to actually turned on UA Widget: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3a7653a27bac54e713a6f69f0623e7c1798f3b

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #47) > Anyway, I think for now the safest thing to do is the attached patch, and > then once we do bug 1489308 we can change this code to assert there is no > loadgroup on entry, maybe. Would need to check. Could you file a followup > bug, depending on bug 1489308, to do that? Filed bug 1492227.

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c77cf5e040b6 Part I, Restore the video controls accessible tree r=Gijs

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aeaa06108ea2 Part II, Make error label visible r=Gijs

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a11678be551 Part III, Wait for audio source to load before starting a11y tests r=surkov

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff7afc7ea4b7 Part IV, Don't reset mDocumentLoadGroup if the same LoadGroup is passed r=bzbarsky

Pushed by tchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/80d4b05cbfa9 Part V, Re-enable UA Widget on Desktop Nightly r=smaug

https://hg.mozilla.org/mozilla-central/rev/c77cf5e040b6 https://hg.mozilla.org/mozilla-central/rev/aeaa06108ea2 https://hg.mozilla.org/mozilla-central/rev/9a11678be551 https://hg.mozilla.org/mozilla-central/rev/ff7afc7ea4b7 https://hg.mozilla.org/mozilla-central/rev/80d4b05cbfa9