Closed Bug 1341230 Opened 3 years ago Closed 3 years ago

add nsIDOMWindowUtils methods to add and remove EventStates for an element

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Split off from bug 865401.  We need a way to make an element start and stop matching :past and :future (for WebVTT captions), and the simplest way seems to be to add EventStates bits to represent these, and allow them to be set by calling some methods on nsIDOMWindowUtils from webvtt.jsm.

It sounds like the HTML autofill feature might want this too, for temporary highlighting of which fields will be filled in.
Priority: -- → P2
Comment on attachment 8840722 [details]
Bug 1341230 - Part 1: Rename ESM_MANAGED_STATES to EXTERNALLY_MANAGED_STATES.

https://reviewboard.mozilla.org/r/115164/#review116640

::: dom/base/Element.h:518
(Diff revision 1)
>  
>    // Style state computed from element's state and style locks.
>    EventStates StyleStateFromLocks() const;
>  
>  protected:
>    // Methods for the ESM to manage state bits.  These will handle

We might want to replace ESM with something else.
Comment on attachment 8840722 [details]
Bug 1341230 - Part 1: Rename ESM_MANAGED_STATES to EXTERNALLY_MANAGED_STATES.

https://reviewboard.mozilla.org/r/115164/#review116878
Attachment #8840722 - Flags: review?(bugs) → review+
Comment on attachment 8840723 [details]
Bug 1341230 - Part 2: Add C++ API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115166/#review116880

Don't know yet how this will be used, but I guess part 3 shows that.
Attachment #8840723 - Flags: review?(bugs) → review+
Comment on attachment 8840724 [details]
Bug 1341230 - Part 3: Add nsIDOMWindowUtils API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115168/#review116882

::: dom/base/nsDOMWindowUtils.cpp:4154
(Diff revision 1)
> +  EventStates state = GetEventStateForString(aStateString);
> +  if (state.IsEmpty()) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  EventStateManager::AddManuallyManagedStates(content->AsElement(), state);

Aha, we just call ESM here which forwards to element. So the methods don't need to be in ESM.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1989
(Diff revision 1)
>  
> +  /**
> +   * Adds an EventStates bit to the element.
> +   *
> +   * The state string must be one of the following:
> +   *   * (none yet; but for example "higlighted" for NS_EVENT_STATE_HIGHLIGHTED)

Why string based API? Why not some consts in nsIDOMWindowUtils?
But both are fine to me. Using consts would make the code a bit simpler and faster though
Attachment #8840724 - Flags: review?(bugs) → review-
Comment on attachment 8840723 [details]
Bug 1341230 - Part 2: Add C++ API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115166/#review116884

::: dom/events/EventStateManager.cpp:5955
(Diff revision 1)
>        return 0;
>    }
>  }
>  
> +/* static */ void
> +EventStateManager::AddManuallyManagedStates(Element* aElement,

Why these methods are in ESM? Why not just in Element
Attachment #8840723 - Flags: review+ → review-
Comment on attachment 8840724 [details]
Bug 1341230 - Part 3: Add nsIDOMWindowUtils API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115168/#review116882

> Aha, we just call ESM here which forwards to element. So the methods don't need to be in ESM.

Yeah, makes sense to just move them to Element.

> Why string based API? Why not some consts in nsIDOMWindowUtils?
> But both are fine to me. Using consts would make the code a bit simpler and faster though

Initially I thought of suggesting to add corresponding consts like |const uint64_t NS_EVENT_STATE_HIGHLIGHTED = ...| here, but (a) you can't just set it equal to the C++ const, you'd need to include a literal 0x10000000 value (after having expanded out NS_DEFINE_EVENT_STATE_MACRO mentally), (b) I would worry about them getting out of sync, and (c) all uint64_t const values couldn't fit in a JS Number.  I could have suggested adding unrelated consts, like STATE_HIGHLIGHTED = 0x1, I guess.  Anyway, I find string-enum-based JS APIs easier to use, and I don't think it being a little slower than numeric constants should be an issue here.
Comment on attachment 8840723 [details]
Bug 1341230 - Part 2: Add C++ API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115166/#review116996
Attachment #8840723 - Flags: review?(bugs) → review+
Comment on attachment 8840724 [details]
Bug 1341230 - Part 3: Add nsIDOMWindowUtils API to add/remove manually managed EventStates bits.

https://reviewboard.mozilla.org/r/115168/#review116998

::: dom/base/nsDOMWindowUtils.cpp:4144
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsDOMWindowUtils::AddManuallyManagedState(nsIDOMElement* aElement,
> +                                          const nsAString& aStateString)
> +{
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);

You could QI to Element, then you wouldn't need to call AsElement() later

::: dom/base/nsDOMWindowUtils.cpp:4162
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsDOMWindowUtils::RemoveManuallyManagedState(nsIDOMElement* aElement,
> +                                             const nsAString& aStateString)
> +{
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);

ditto
Attachment #8840724 - Flags: review?(bugs) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41bd2eefc823
Part 1: Rename ESM_MANAGED_STATES to EXTERNALLY_MANAGED_STATES. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f879b73eb504
Part 2: Add C++ API to add/remove manually managed EventStates bits. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0a4869ea6249
Part 3: Add nsIDOMWindowUtils API to add/remove manually managed EventStates bits. r=smaug
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbdc5fc3db27
Backed out changeset 0a4869ea6249 for build bustages
https://hg.mozilla.org/integration/autoland/rev/06734b1a5436
Backed out changeset f879b73eb504 
https://hg.mozilla.org/integration/autoland/rev/95e0ba5f61da
Backed out changeset 41bd2eefc823
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f962c5a62d3
Part 1: Rename ESM_MANAGED_STATES to EXTERNALLY_MANAGED_STATES. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5bef5fcbac2f
Part 2: Add C++ API to add/remove manually managed EventStates bits. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b1d9e7a573c8
Part 3: Add nsIDOMWindowUtils API to add/remove manually managed EventStates bits. r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.