Nika Layzell [:nika] (ni? for response) Assignee
	Description • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 2 • 9 years ago

Updated.

	(no longer active)
	Comment 3 • 9 years ago

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.

	Pulsebot
	Comment 4 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9aea0c4a0822

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 5 • 9 years ago

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

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8853d35b1154

	Phil Ringnalda (:philor)
	Comment 7 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/8853d35b1154

	Jean-Yves Perrier [:teoli]
	Comment 8 • 9 years ago

Adding dev-doc-needed to make this explicit in the doc

	Jean-Yves Perrier [:teoli]
	Comment 9 • 9 years ago

> -                [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)?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 10 • 9 years ago

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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 11 • 9 years ago

I've filed bug 1231193 to fix this, thanks!