Closed Bug 1496242 Opened 6 years ago Closed 6 years ago

Convert datetimebox to UA Widget

Categories

(Core :: XBL, task, P3)

Product:
Core
Component:
XBL
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(3 files)

Bug 1496242 - Part I, Simplify nsIDateTimeInputArea interface
46 bytes, text/x-phabricator-request
Details | Review
Bug 1496242 - Part II, Convert datetimebox to UA Widget
46 bytes, text/x-phabricator-request
Details | Review
Bug 1496242 - Part III, nsFocusManager should account for UA Widget when deciding to traverse into NAC
46 bytes, text/x-phabricator-request
Details | Review 
This bug covers work needed to convert datetimebox to UA Widget.

We would need an alternative to bug 1456833 here...
Priority: -- → P3
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.
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
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.
(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.
See Also: → 1495621
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
Have you measured the performance of creating <input type=date>?
Flags: needinfo?(timdream)
(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.
Flags: needinfo?(timdream)
running any performance tests on a debug build is useless ;)

Want to retry on an opt build?
Flags: needinfo?(timdream)
(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.
(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?
(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.
(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.
Flags: needinfo?(timdream)
Attachment #9018113 - Attachment description: Bug 1496242 - Part I, Remove nsIDateTimeInputArea interface → Bug 1496242 - Part I, Simplify nsIDateTimeInputArea interface
(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
Flags: needinfo?(timdream)
(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.
Flags: needinfo?(timdream)
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.
There is a point to compare. Microbenchmarks, Speedometer as an example, create <input> elements in a loop basically.
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
(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.
(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.
Well, that is not the point. Microbenchmarks don't do that, they just loop.
Pushed by tchien@mozilla.com:
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
Depends on: 1514040
Type: enhancement → task
Regressions: 1567191
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: