1496242 - Convert datetimebox to UA Widget

Status: RESOLVED FIXED

(Core :: XBL, task, P3)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This bug covers work needed to convert datetimebox to UA Widget.

We would need an alternative to bug 1456833 here...

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1496242 - Part I, Simplify nsIDateTimeInputArea interface

This patch removes the nsIDateTimeInputArea interface, implemented by the
datetime bindings, with the following:

- hasBadInput() is re-implemented in C++ in nsIDateTimeControlFrame since
  C++ needs the return value synchronously.
- SetValueFromPicker() and SetPickerState() are avoided completed since they
  are simply called by HTMLInputElement methods exposed to the frame
  script. They are avoided by dispatching events from the frame script directly.
- Other methods are replaced by dispatching events.

Care has been taken to ensure events are dispatched to the anonymous
<xul:datetimebox>, to make sure the events cannot be heard by web content.
This is not necessary once we upgrade to UA Widget since event dispatches to
Shadow DOM does not get re-dispatched to the document.

The earlier version of this patch was proposed in bug 1456833. The bug was
WONTFIXED because we were under the impression that bug 1461742 will supersede
it. Yet bug 1461742 will only work with Custom Elements in chrome documents,
so we will have to fall back to events, as what we have done with videocontrols.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1496242 - Part II, Convert datetimebox to UA Widget

This patch converts datetimebox.xml to datetimebox.js and loads it as an UA Widget,
while touches things here and there to make it work.

In HTMLInputElement manages the lifecycle of the datetimebox UA Widget.
It is loaded when in <input> has type date or time, or have its type switch to date or time.

AttachAndSetUAShadowRoot() is moved from HTMLMediaElement to nsGenericHTMLElement,
so we could share the implementation.

nsDateTimeControlFrame is changed so that when UA Widget is enabled,
it would not generate <xul:datetimebox>.

Like bug 1483972, a check is added in nsCSSFrameConstructor::CreateGeneratedContentItem()
to make sure we don't generate pseudo content inside <input>.

Assertions in IntlUtils is changed to allow UAWidget to call the methods.

Depends on D9056

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The two patches almost fix everything, except for dom/base/test/test_bug1453693.html

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b494dcffdeea0606b444aecd0a6aacfcc2764d

The test case here does not confirm the way nsFocusManager tab into Shadow DOM, or perhaps nsFocusManager is confused by a focusable shadow root host (this can happen in web content too though). Either way, this needs a separate in-depth patch. I can already a few cases with Shadow DOM where the tab order is quite strange.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)
> The test case here does not confirm the way nsFocusManager tab into Shadow
> DOM, or perhaps nsFocusManager is confused by a focusable shadow root host
> (this can happen in web content too though). Either way, this needs a
> separate in-depth patch. I can already a few cases with Shadow DOM where the
> tab order is quite strange.

It may not be the same issue, but we just saw some other Shadow DOM / nsFocusManager issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1495621#c5.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1496242 - Part III, nsFocusManager should account for UA Widget when deciding to traverse into NAC

In bug 1430692, a special case was added in nsFocusManager::GetNextTabbableContentInScope()
that traverse into the element when the primany frame implements nsIAnonymousContentCreator,
assuming there will be NAC to focus within.

This is not the case when UA Widget is turned on, where the focusable inner inputs will
be put in the UA Widget Shadow DOM.

The change here account this fact.

Depends on D9057

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Have you measured the performance of creating <input type=date>?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Have you measured the performance of creating <input type=date>?

Running 

    let el = document.createElement('input');
    el.type = 'date';
    document.body.appendChild(el);
    el.offsetHeight;

1000 times, it is slightly slower (~25%+) on my debug build, comparing XBL implementation and UA Widget implementation.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

running any performance tests on a debug build is useless ;)

Want to retry on an opt build?

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Want to retry on an opt build?

will do.

==

So :smaug's review comment makes me realized my incorrect assumption of bubbles=false events: they are still accessible to capture event listeners on the parent.

On the flip side, composed=false events dispatch to Shadow DOM does not get dispatched to the document.

Given that, in order to prevent leaking events with the XBL implementation, the patches need to be altered so that we will continue to use nsIDateTimeInputArea for the XBL binding, and only switch to events on UA Widget. It would be more complex than I like it to be.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > Want to retry on an opt build?
> 
> will do.
> 
> ==
> 
> So :smaug's review comment makes me realized my incorrect assumption of
> bubbles=false events: they are still accessible to capture event listeners
> on the parent.
> 
> On the flip side, composed=false events dispatch to Shadow DOM does not get
> dispatched to the document.
> 
> Given that, in order to prevent leaking events with the XBL implementation,
> the patches need to be altered so that we will continue to use
> nsIDateTimeInputArea for the XBL binding, and only switch to events on UA
> Widget. It would be more complex than I like it to be.

When can we drop the XBL implementations and only use UA widgets? Does that depend on the main shadow dom pref being removed?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> When can we drop the XBL implementations and only use UA widgets? Does that
> depend on the main shadow dom pref being removed?

It would depend on that, and the removal of UA Widget pref.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #8)
> running any performance tests on a debug build is useless ;)

Test on opt build shows that it is 75% slower.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Patch updated:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cf261a4f0fd3929893b9c4cc2af5c49106d012c

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > running any performance tests on a debug build is useless ;)
> 
> Test on opt build shows that it is 75% slower.

UAWidget being slower?


Also, test also without offsetHeight

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #14)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #12)
> > (In reply to Olli Pettay [:smaug] from comment #8)
> > > running any performance tests on a debug build is useless ;)
> > 
> > Test on opt build shows that it is 75% slower.
> 
> UAWidget being slower?
> 
> 
> Also, test also without offsetHeight

Yes, UA Widget is slower.

The XBL binding is not actually attached without a reflow so there is no point comparing that with UA Widget.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This is the profile of the current patches w/o offsetHeight

https://perfht.ml/2PTiHEq

there are rooms to improve if we need want to do it.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

There is a point to compare. Microbenchmarks, Speedometer as an example, create <input> elements in a loop basically.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With the removal of the getLocaleString() method I added the performance is now comparable to the XBL implementation.

https://phabricator.services.mozilla.com/D9057?vs=27621&id=27966&whitespace=ignore-most#toc

https://perfht.ml/2SbCVL0

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #17)
> There is a point to compare. Microbenchmarks, Speedometer as an example,
> create <input> elements in a loop basically.

By that standard, we are regressed by 1000x, since XBL didn't really do the work of creating a functional datetime widget until reflow.

I don't think we can do anything about it without contracting one of the foundational goals of XBL removal.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19)
> (In reply to Olli Pettay [:smaug] from comment #17)
> > There is a point to compare. Microbenchmarks, Speedometer as an example,
> > create <input> elements in a loop basically.
> 
> By that standard, we are regressed by 1000x, since XBL didn't really do the
> work of creating a functional datetime widget until reflow.
> 
> I don't think we can do anything about it without contracting one of the
> foundational goals of XBL removal.

Yeah, we ran into similar issues when testing stuff with <marquee> (where we actually found properties weren't assigned to the node until after the frame construction happens): https://bugzilla.mozilla.org/show_bug.cgi?id=1425874#c7. I guess we could test this by starting a timer, doing the element creation in a loop in an inline script and then waiting for `onload` and ending the timer. That would ensure both cases are initialized without forcing a layout flush, and more closely replicate pages that have these in their markup.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Well, that is not the point. Microbenchmarks don't do that, they just loop.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Review comments addressed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63480ff4bc1ae8d33476d3987514fb1948ce997f

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Review comments addressed, hopefully correctly this time.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1387262d2f93
Part I, Simplify nsIDateTimeInputArea interface r=mconley,smaug
https://hg.mozilla.org/integration/autoland/rev/66df568f60f6
Part II, Convert datetimebox to UA Widget r=dholbert,jaws,smaug
https://hg.mozilla.org/integration/autoland/rev/4ec5f59ab723
Part III, nsFocusManager should account for UA Widget when deciding to traverse into NAC r=smaug

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1387262d2f93
https://hg.mozilla.org/mozilla-central/rev/66df568f60f6
https://hg.mozilla.org/mozilla-central/rev/4ec5f59ab723