Closed Bug 1196479 Opened 4 years ago Closed 4 years ago

Fire selectstart and selectionchange events on the input node when the selection in that editor changes

Categories

(Core :: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

If https://github.com/w3c/selection-api/issues/53 is accepted, then we need to handle input nodes in that way. This patch should handle using them in this way.
Assignee: nobody → michael
Depends on: 571294
Comment on attachment 8655049 [details] [diff] [review]
Fire selectstart and selectionchange events on the input node when the selection in that editor changes

Review of attachment 8655049 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsTextEditorState.cpp
@@ +92,5 @@
>      if (!mTextEditorState) {
>        return NS_OK;
>      }
>  
> +    mozilla::dom::AutoHideSelectionChanges hideSelectionChanges

You don't need mozilla::dom, the translation unit is using that namespace.

@@ +1250,5 @@
>      // Do not initialize the editor multiple times.
>      return NS_OK;
>    }
>  
> +  mozilla::dom::AutoHideSelectionChanges hideSelectionChanges(GetConstFrameSelection());

Same.

::: layout/forms/nsTextControlFrame.cpp
@@ +260,5 @@
>    // Make sure that editor init doesn't do things that would kill us off
>    // (especially off the script blockers it'll create for its DOM mutations).
>    {
> +    nsCOMPtr<nsITextControlElement> txtCtrl = do_QueryInterface(GetContent());
> +    NS_ASSERTION(txtCtrl, "Content not a text control element");

Nit: please change this to MOZ_ASSERT while you're here.

@@ +264,5 @@
> +    NS_ASSERTION(txtCtrl, "Content not a text control element");
> +
> +    // Hide selection changes during the initialization, as webpages should not
> +    // be aware of these initializations
> +    mozilla::dom::AutoHideSelectionChanges hideSelectionChanges(txtCtrl->GetConstFrameSelection());

Nit: dom::

::: layout/generic/Selection.h
@@ +324,5 @@
>    bool mApplyUserSelectStyle;
> +
> +  // Non-zero if we don't want any changes we make to the selection to be
> +  // visible to content. If non-zero, content won't be notified about changes.
> +  uint32_t mHideChanges;

Hmm, how about a slightly better name such as mSelectionChangeBlockerCount, and AddSelectionChangeBlocker(), RemoveSelectionChangeBlocker() and IsBlockingSelectionChangeEvents()?

@@ +354,5 @@
> +{
> +private:
> +  nsRefPtr<Selection> mSelection;
> +public:
> +  explicit AutoHideSelectionChanges(const nsFrameSelection* aFrame);

Please use the MFBT GuardObject.h macros for this.

@@ +358,5 @@
> +  explicit AutoHideSelectionChanges(const nsFrameSelection* aFrame);
> +
> +  explicit AutoHideSelectionChanges(Selection* aSelection)
> +  {
> +    mSelection = aSelection;

Nit: this goes in the initializer list.

::: layout/generic/nsSelection.cpp
@@ +6030,5 @@
> +  mHideChanges--;
> +}
> +
> +bool
> +Selection::IsHidingChanges()

Nit: const.
Attachment #8655049 - Flags: review?(ehsan) → review+
This got backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1cec2d7bf26b because of TEST-UNEXPECTED-FAIL | /eventsource/eventsource-close.htm | EventSource: close(), test events - assert_unreached: Dunno what to do with message number 3 Reached unreachable code
https://hg.mozilla.org/mozilla-central/rev/8853d35b1154
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Adding dev-doc-needed to make this explicit in the doc
Keywords: dev-doc-needed
> -                [Pref="dom.select_events.enabled"]
> -                attribute EventHandler onselectionchange; 

I'm trying to document this (by updated the selectionchange and selectstart event documentation), but I don't see the rationale to remove Document.onselectionchange property that this patch also does. Does that mean that this property has been removed (and will therefore never reach a release version of Gecko)?
Flags: needinfo?(michael)
That looks like a mistake to me... I don't have time to look over it right now, but onselectionchange should definitely be available on document I would think.

Leaving the ni? so that I'll get around to this next week.
I've filed bug 1231193 to fix this, thanks!
Blocks: 1231193
Flags: needinfo?(michael)
Depends on: 1248148
Depends on: 1242718
Blocks: 571294
No longer depends on: 571294
Blocks: 1309612
No longer blocks: 1309612
See Also: → 1309628
You need to log in before you can comment on or make changes to this bug.