Closed Bug 1456833 Opened 7 years ago Closed 6 years ago

Remove nsIDateTimeInputArea interface

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file)

Bug 1456833 - Remove nsIDateTimeInputArea interface
59 bytes, text/x-review-board-request
smaug
: review-
mconley
: review+
Details 
This interface is used to call XBL methods from C++ so it could talk to datetimebox.xml, which implements the interface. We should replace it with events in system event group, as we will unlikely be able to support this interface with whatever we are replacing this with, like bug 1431255.
Priority: -- → P5
Assignee: nobody → timdream
Status: NEW → ASSIGNED
My patch as it is written now hits a lot of unsafe assertion warning error.

https://searchfox.org/mozilla-central/source/dom/events/EventDispatcher.cpp#757

I am not familiar with the concept enough to tell why it is ok to call into nsIDateTimeInputArea in the same place, but not ok via the event dispatcher. I can replace them all with script runner runnable I think, except for the HasBadInput() call, but I prefer to understand more there since making code async might create more problems down the road.
(the correct line should be https://searchfox.org/mozilla-central/rev/53afcfdbabed96883126d0ebbcac499b358e32f2/dom/events/EventDispatcher.cpp#773 since it's dispatched on the node)
Attachment #8973322 - Flags: review?(mconley)
The assertion errors were caused by the call sites where I dispatch MozDateTimeValueChanged/MozNotifyMinMaxStepAttrChanged/MozCheckForBadInput events. I ended up deferred the *Changed events to a runnable, and implement HasBadInput() in C++.

I was a bit worry deferring the *Changed actions could cause any side effects, but as it didn't show up on the tests, I am not too worry about it.

Let me know if this makes sense.
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

https://reviewboard.mozilla.org/r/241798/#review249018

Thanks, timdream! I want to look more closely at this, so keeping r?, but for now, see the feedback below.

::: toolkit/content/browser-content.js:1866
(Diff revision 4)
> +    this._inputElement.dispatchEvent(
> +      new win.CustomEvent("MozSetDateTimePickerState",
> +        { detail: false }, win));

Is this necessary to ultimately remove the XBL binding? It seems more ergonimic to just use WebIDL to interact with the underlying frame rather than firing events. I don't feel crazy strongly about it, but I'm wondering if this is a design decision distinct from getting rid of the nsIDateTimeInputArea IDL file. Or is this a stepping stone to a different mechanism for talking to this frame?

::: toolkit/content/widgets/datetimebox.xml:1227
(Diff revision 4)
>      </content>
>  
> -    <implementation implements="nsIDateTimeInputArea">
> +    <implementation>
>        <constructor>
>        <![CDATA[
> -        this.DEBUG = false;
> +        this.DEBUG = true; // XXX unset before check-in

Don't forget! :)

::: toolkit/content/widgets/datetimebox.xml:1342
(Diff revision 4)
>            // Set property as well for convenience.
>            field.disabled = this.mInputElement.disabled;
>            field.readOnly = this.mInputElement.readOnly;
>            field.setAttribute("aria-label", aLabel);
>  
> +          // Used to store the non-formatted value, clered when value is

Typo: clered -> cleared
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

https://reviewboard.mozilla.org/r/241798/#review249018

> Is this necessary to ultimately remove the XBL binding? It seems more ergonimic to just use WebIDL to interact with the underlying frame rather than firing events. I don't feel crazy strongly about it, but I'm wondering if this is a design decision distinct from getting rid of the nsIDateTimeInputArea IDL file. Or is this a stepping stone to a different mechanism for talking to this frame?

The code path here was: browser-content.js (JS) -> HTMLInputElement (C++) -> nsDateTimeControlFrame (C++) -> nsIDateTimeInputArea (JS). It just don't feel right to me that we have two JS instances talking to each other via two C++ classes.

The event cannot be avoided anyway since with nsIDateTimeInputArea gone the only way nsDateTimeControlFrame could talk to the frame is to firing an event (probably of the same name).
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

https://reviewboard.mozilla.org/r/241798/#review249018

> The code path here was: browser-content.js (JS) -> HTMLInputElement (C++) -> nsDateTimeControlFrame (C++) -> nsIDateTimeInputArea (JS). It just don't feel right to me that we have two JS instances talking to each other via two C++ classes.
> 
> The event cannot be avoided anyway since with nsIDateTimeInputArea gone the only way nsDateTimeControlFrame could talk to the frame is to firing an event (probably of the same name).

Sorry, I meant "the only way nsDateTimeControlFrame could take to *the binding* is to firing an event"
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

https://reviewboard.mozilla.org/r/241798/#review249620

Okay, this seems okay - at least the XBL binding changes. You might want dholbert to sign-off on the layout changes, and smaug to sign-off on the DOM changes.

Thanks!

::: dom/webidl/HTMLInputElement.webidl
(Diff revision 4)
>  
>  partial interface HTMLInputElement {
>    [Pref="dom.forms.datetime", ChromeOnly]
>    DateTimeValue getDateTimeInputBoxValue();
>  
> -  [Pref="dom.forms.datetime", ChromeOnly]

I think you're going to need a DOM peer to sign-off on this. smaug, I think, helped to get this stuff landed originally, so he might be a good choice if he has the cycles.

::: layout/forms/nsDateTimeControlFrame.cpp:96
(Diff revision 4)
> -  nsCOMPtr<nsIDateTimeInputArea> inputAreaContent =
> -    do_QueryInterface(mInputAreaContent);
> -  if (inputAreaContent) {
> -    inputAreaContent->FocusInnerTextBox();
> +  nsContentUtils::DispatchTrustedEvent(mContent->GetComposedDoc(),
> +                                       mInputAreaContent,
> +                                       NS_LITERAL_STRING("MozFocusInnerTextBox"),
> +                                       false, false);

Out of curiosity, why are we dispatching these events immediately instead of using AddScriptRunner (which would do the same thing if it's safe to run script) ?

::: layout/forms/nsDateTimeControlFrame.cpp:429
(Diff revision 4)
> -              contentAsInputElem->GetAttr(aNameSpaceID, aAttribute, value);
> +            contentAsInputElem->GetAttr(aNameSpaceID, aAttribute, value);
> -              inputAreaContent->SetEditAttribute(name, value);
> +            mInputAreaContent->SetAttr(kNameSpaceID_None, aAttribute, value, true);
> -            }
>            }
> +          RefPtr<Runnable> event =
> +            new DispatchDateTimeEvent(mInputAreaContent, 

Nit: trailing whitespace

::: toolkit/content/widgets/datetimebox.xml:1533
(Diff revision 4)
>              document.getAnonymousElementByAttribute(this, "anonid", "edit-wrapper");
>  
>            for (let child = editRoot.firstChild; child; child = child.nextSibling) {
>              if ((child instanceof HTMLSpanElement) &&
>                  child.classList.contains("datetime-edit-field")) {
> -              child.removeAttribute(aName);
> +              for (let attrName of values.keys()) {

Note that you can get keys and values from Maps like:

```js
for (let [attrName, attrValue] of values) {
  child.setAttribute(attrName, attrValue);
}
```
Attachment #8973322 - Flags: review?(mconley) → review+
Attachment #8973322 - Flags: review?(dholbert)
Attachment #8973322 - Flags: review?(bugs)
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

mconley, sorry about having you to review this again; I made a few further clean-up in updateEditAttribute and SyncDisabledStateEvent(), let me know if this is alright.
Attachment #8973322 - Flags: review+ → review?(mconley)
Attachment #8973322 - Flags: review?(mconley)
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

https://reviewboard.mozilla.org/r/241798/#review249786

JS/XBL changes LGTM. Thanks!

::: layout/forms/nsDateTimeControlFrame.cpp:405
(Diff revisions 4 - 6)
>  
>  void
>  nsDateTimeControlFrame::ContentStatesChanged(EventStates aStates)
>  {
>    if (aStates.HasState(NS_EVENT_STATE_DISABLED)) {
> -    nsContentUtils::AddScriptRunner(new SyncDisabledStateEvent(this));
> +    SyncDisabledState();

Seems okay, assuming now is an okay time to modify the DOM.
Attachment #8973322 - Flags: review?(mconley) → review+
In principal I don't like this approach, since it is rather heavy. Dispatching DOM events is way slower than couple of method calls.
(In reply to Olli Pettay [:smaug] from comment #17)
> In principal I don't like this approach, since it is rather heavy.
> Dispatching DOM events is way slower than couple of method calls.

Any suggestions to alternatives that don't involve implementing an interface in JS?
Flags: needinfo?(bugs)
It is unclear to me why we want this change. Eventually we'll have C++ shadow DOM for date stuff, and then there won't be need for the JS which is currently in XBL, but it will all be in C++.
Nor will be there need for any C++-only interfaces.

Perhaps I'm not aware of some plans.
Flags: needinfo?(bugs)
I'd rather move the XBL methods one by one to C++ and eventually there wasn't really for anything else in XBL but the <xbl:content> which could be later replaced with shadow DOM. Use of the interface would disappear while doing that all, and without need to use events as a temporary way to call methods.
Is there some particular reason why we want to remove use of the interface now?
Comment on attachment 8973322 [details]
Bug 1456833 - Remove nsIDateTimeInputArea interface

Hmm, MozReview is behaving oddly.
I was going to r- for now, since I don't understand why we want to do this now - making the architecture more complicated temporarily.

But feel free to explain and convince me why we want to do this.
Attachment #8973322 - Flags: review?(bugs) → review-
The plan (involving bug 1431255) is to have a shadow DOM hosting the JS converted from XBL. There is no plan to rewrite it to C++. This is effectively a blocker to bug 1431255.
Sidenote: we generally want to remove `implements=nsIDOMXUL*` from all XBL bindings (even for those in the parent process), since there's no analog with Custom Elements as per https://bugzilla.mozilla.org/show_bug.cgi?id=1404420#c15. If you have ideas on how we could continue to use it or have an alternative in mind it would make migration easier since there's still a bunch of consumers: https://bgrins.github.io/xbl-analysis/tree/#implements.
See Also: → 1461742
I am told bug 1461742 should support this use case, making the approach here obsolete.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Attachment #8973322 - Flags: review?(dholbert)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> My patch as it is written now hits a lot of unsafe assertion warning error.
> 
> https://searchfox.org/mozilla-central/source/dom/events/EventDispatcher.
> cpp#757
> 
> I am not familiar with the concept enough to tell why it is ok to call into
> nsIDateTimeInputArea in the same place, but not ok via the event dispatcher.
> I can replace them all with script runner runnable I think, except for the
> HasBadInput() call, but I prefer to understand more there since making code
> async might create more problems down the road.

So, just for the record, it's *not* entirely safe to call into nsIDateTimeInputArea at this point, and that should probably be fixed. But there are different degrees of unsafety.

Calling a chrome-implemented method with well-defined behavior is relatively low-risk. We should really not be calling any JS during layout, but the better we can understand the scope of the JS that a given operation runs, the less risky it is.

Dispatching a DOM event is inherently riskier. That requires touching a bunch of DOM state info, which is already troublesome. But it can also run arbitrary JS, and it's very easy to wind up adding listeners that run unsafe JS without noticing.

Dispatching a DOM event to a non-chrome document, though, (which your patch does) is a whole different ballpark. That allows arbitrary content JS to run. And if it runs during, say, layout, it has the opportunity to break arbitrary invariants of our layout code, and can almost certainly do so exploitably.
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: