Closed
Bug 1341230
Opened 7 years ago
Closed 7 years ago
add nsIDOMWindowUtils methods to add and remove EventStates for an element
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
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.
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-review |
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 6•7 years ago
|
||
mozreview-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 7•7 years ago
|
||
mozreview-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 8•7 years ago
|
||
mozreview-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-
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-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/#review116996
Attachment #8840723 -
Flags: review?(bugs) → review+
Comment 14•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
sorry had to back this out for build bustages like https://treeherder.mozilla.org/logviewer.html#?job_id=81778121&repo=autoland&lineNumber=12915
Comment 20•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
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
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f962c5a62d3 https://hg.mozilla.org/mozilla-central/rev/5bef5fcbac2f https://hg.mozilla.org/mozilla-central/rev/b1d9e7a573c8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•