Closed Bug 1318312 Opened 8 years ago Closed 7 years ago

Selecting an element inside an element with contenteditable="true" does not focus the contenteditable element

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
platform-rel --- +
firefox55 --- fixed

People

(Reporter: denschub, Assigned: masayuki)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, Whiteboard: [webcompat][platform-rel-Facebook])

Attachments

(5 files, 1 obsolete file)

Simple testcase
1.18 KB, text/html
Details
Bug 1318312 part.1 Add automated tests for checking focus move at using Selection API
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1318312 part.2 Mark Selection as "called by JS" when every Selection API which may cause changing selection is called by JS
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS
59 bytes, text/x-review-board-request
smaug
: review+
Details
testcase.html
392 bytes, text/html
Details 
Given the following HTML

> <div contenteditable="true">
>   <span>text</span>
> </div>

When selecting the text inside the <span> using the Selection API, the <div> does not get focused and the content is not immediately editable. I'll attach a test case in a second.

While this issue can be worked around by manually setting the focus on the <div> before selecting the contents, Chrome, Safari, and Edge automatically focus the <div>, so we should do as well.
Attached file Simple testcase — 
Test case as mentioned attached. The button labeled "select without focus (not working in Firefox)" shows the issue, the other button was added for verifying the workaround.
platform-rel: --- → ?
Whiteboard: [webcompat] → [webcompat][platform-rel-Facebook]
This is well known issue of past years.  We have several issue that related to this.
Priority: -- → P2
platform-rel: ? → +
Rank: 15
(In reply to Makoto Kato [:m_kato] from comment #2)
> This is well known issue of past years.  We have several issue that related
> to this.

Can you point us to some of those bugs, please? Thanks!
Flags: needinfo?(m_kato)
Neil, you know, Gecko doesn't move focus on comment #0 and comment #1 case.  Should we change focus behavior like other browsers (Edge, WebKit and Blink) for web compatibility?
Flags: needinfo?(m_kato) → needinfo?(enndeakin)
If other browsers are doing this, it seems reasonable to do.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #5)
> If other browsers are doing this, it seems reasonable to do.

Chromium does it. And Edge doesn't work well but it tries to do it (at least looks like).
https://jsfiddle.net/d_toybox/e752ntu8/
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Created automated tests. I'll try to fix this bug next week, but I'm not sure if these behaviors are reasonable for our current implementation.
# I wonder, I could write the tests as web platform tests, though. However, the behavior hasn't been defined by any specs yet. So, Selection API or something should define the behavior strictly first. (I don't think we shouldn't fix this bug until then.)
Hmm, it's difficult to distinguish if a selection change is occurred by:

* a user operation like arrow keys
* an edit action
* called by selection API
* or something others

Therefore, I'm struggling with restricting to run adjusting focus... It seems that we need to redesign how to post selection change reasons.
Okay, perhaps, I succeeded to create the fix. I'm taking this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Only with some paths, JS cannot move selection range outside of focused editing host due to ancestor limit.  However, it's possible with some ways, for example, using Selection.removeAllRanges() and Selection.addRange().  Additionally, this is an incompatible behavior with Chrome. So, I think that renaming todos in test_focus_move_by_selection_API.html should be fixed in follow up bug.
And I didn't write the tests as web-platform tests because no spec declare focus moving. Although, I'll post an issue to Selection API, but perhaps, focus behavior can depend on implementations due to conforming to platform manners.

Chromium implements contenteditable similar to Gecko. It needs focus for modifying the content. Therefore, we should move focus for compatibility with other browsers. On the other hand, Edge can modify even without focus. Edge just refers selection range to modify its content. So, I think that it's difficult to standardize this behavior.
Attachment #8836070 - Attachment is obsolete: true
Comment on attachment 8839896 [details]
Bug 1318312 part.1 Add automated tests for checking focus move at using Selection API

https://reviewboard.mozilla.org/r/114482/#review117950

This is mostly rs+. I assume you've run this also in other browsers and they give similar results, at least blink (since we're now trying to follow their model)

::: layout/generic/test/test_moving_focus_by_selection_API.html:1
(Diff revision 1)
> +<!DOCTYPE>

Making this a wpt test would be really nice, even if it was still in testing/web-platform/mozilla/tests .

::: layout/generic/test/test_moving_focus_by_selection_API.html:45
(Diff revision 1)
> +  }
> +}
> +
> +function getStartPoint(aElement)
> +{
> +  var p = aElement.tagName == "P" || aElement.tagName == "A" ? aElement : aElement.firstChild;

Some comment why P and A are special cased here would make reading this easier.

::: layout/generic/test/test_moving_focus_by_selection_API.html:57
(Diff revision 1)
> +  return { node: p.firstChild, offset: p.firstChild.length };
> +}
> +
> +// Helper methods to test Range
> +
> +function collapseOfRange(aDescription, aIndex, aToStart, aElementToGetFocus, aToDo)

Call this just collapseRange? or collapseSelectionRange ?
Attachment #8839896 - Flags: review?(bugs) → review+
Comment on attachment 8839897 [details]
Bug 1318312 part.2 Mark Selection as "called by JS" when every Selection API which may cause changing selection is called by JS

https://reviewboard.mozilla.org/r/114484/#review117964

I don't understand the naming "External" in Range and JS. Apparently it means calls from JS. So why not just add *JS methods.

I don't yet understand why this all is needed, but perhaps the next patch explains. But I'd like to see renaming. And if possible, don't add anything to Bindings.conf but to the relevant .webidl.
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#BinaryName

::: dom/base/nsRange.h:347
(Diff revision 1)
>                                     size_t aRangeEnd);
>  
> +  class MOZ_RAII AutoCalledFromExternalSetter final
> +  {
> +  private:
> +    RefPtr<nsRange> mRange;

Do we need RefPtr here if the class is stack only.
Extra addref/release will show in the microbenchmarks.

::: dom/base/nsRange.h:348
(Diff revision 1)
>  
> +  class MOZ_RAII AutoCalledFromExternalSetter final
> +  {
> +  private:
> +    RefPtr<nsRange> mRange;
> +    bool mOldValue;

and this looks rather simple... couldn't you just use AutoRestore like what you do with Selection.
Attachment #8839897 - Flags: review?(bugs) → review-
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review117974

I think I'd like to see a new patch with those small issues fixed.

::: layout/generic/Selection.h:385
(Diff revision 1)
> +   *      <div id="c" contenteditable>
> +   *        de[sc]endant
> +   *  in this case, this returns nullptr because the second range is in
> +   *  non-editable area.
> +   */
> +  Element* GetCommonEditingHost();

Since this method has rather unexpected behavior when there are ranges in non-editable areas, could we call this something like
GetCommonEditingHostForAllRanges()

And what happens if there is a selection range outside top level content editable? I assume this returns null? Want to add a comment about it.

::: layout/generic/nsSelection.cpp:6317
(Diff revision 1)
> +  for (auto& rangeData : mRanges) {
> +    auto& range = rangeData.mRange;
> +    if (NS_WARN_IF(!range)) {
> +      continue;
> +    }
> +    nsCOMPtr<nsIContent> commonAncestor =

Could we possibly not use nsCOMPtr here but nsINode* and then nsINode::IsContent() check and AsContent().
Just to avoid extra addref/release in this case when it isn't needed. This is possibly rather hot code, so better to keep it fast.

::: layout/generic/nsSelection.cpp:6367
(Diff revision 1)
>      return NS_OK;//nothing to do
> - 
> +
> +  // When normal selection is changed by Selection API, we need to move focus
> +  // if common ancestor of all ranges are in an editing host.  Note that we
> +  // don't need to move focus *to* the other focusable node because the other
> +  // browsers behave so.

"...because other browsers don't do it either."
or something like that

::: layout/generic/nsSelection.cpp:6371
(Diff revision 1)
> +  // don't need to move focus *to* the other focusable node because the other
> +  // browsers behave so.
> +  if (mSelectionType == SelectionType::eNormal && mCalledFromExternal) {
> +    nsPIDOMWindowOuter* window = GetWindow();
> +    nsIDocument* document = GetDocument();
> +    nsCOMPtr<nsIEditor> editor = do_QueryInterface(GetHTMLEditor());

This is possibly rather hot code, at least in some microbenchmarks. Could GetHTMLEditor at least return already_AddRefed<nsIEditor> ?

::: layout/generic/nsSelection.cpp:6375
(Diff revision 1)
> +    nsIDocument* document = GetDocument();
> +    nsCOMPtr<nsIEditor> editor = do_QueryInterface(GetHTMLEditor());
> +    // If the document is in design mode or doesn't have contenteditable
> +    // element, we don't need to move focus.
> +    if (editor && window && document && !document->HasFlag(NODE_IS_EDITABLE)) {
> +      RefPtr<Element> newEditingHost = GetCommonEditingHost();

Could we assert somewhere here that we aren't doing anything with native anonymous content. All this code should be for contentEditable only.

::: layout/generic/nsSelection.cpp:6378
(Diff revision 1)
> +    // element, we don't need to move focus.
> +    if (editor && window && document && !document->HasFlag(NODE_IS_EDITABLE)) {
> +      RefPtr<Element> newEditingHost = GetCommonEditingHost();
> +      // When all selected ranges are in an editing host, it should take focus.
> +      if (newEditingHost) {
> +        ErrorResult erv;

IgnoredErrorResult

::: layout/generic/nsSelection.cpp:6392
(Diff revision 1)
> +        nsIContent* focusedContent =
> +          fm->GetFocusedDescendant(window, false, getter_AddRefs(focusedWindow));
> +        nsCOMPtr<Element> focusedElement = do_QueryInterface(focusedContent);
> +        if (focusedElement &&
> +            focusedElement == focusedElement->GetEditingHost()) {
> +          ErrorResult erv;

IgnoredErrorResult

::: layout/generic/nsSelection.cpp:6393
(Diff revision 1)
> +          fm->GetFocusedDescendant(window, false, getter_AddRefs(focusedWindow));
> +        nsCOMPtr<Element> focusedElement = do_QueryInterface(focusedContent);
> +        if (focusedElement &&
> +            focusedElement == focusedElement->GetEditingHost()) {
> +          ErrorResult erv;
> +          focusedElement->Blur(erv);

Do other browsers change focus synchronously when range or selection APIs are used? Worth to test.
Attachment #8839898 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8839896 [details]
> Bug 1318312 part.1 Add automated tests for checking focus move at using
> Selection API
> 
> https://reviewboard.mozilla.org/r/114482/#review117950
> 
> This is mostly rs+. I assume you've run this also in other browsers and they
> give similar results, at least blink (since we're now trying to follow their
> model)

Ah, good point. I've not run it on other browsers yet. Although, I don't know how to run mochitest simply on the other browses, I rewrite some points in the test and run on Google Chrome.  Then, I see following failures:
(FYI: I don't run runSelectionRemoveRangeTests because it's not implemented on Blink and runRangeCollapseTests, runRangeSelectNodeTests, runRangeSetEndTests, runRangeSetStartTests because Selection.getRangeAt of Blink returns a copy of range, not a reference to the range of selection.)

Then, I see at least 2 unexpected result.

One is, perhaps due to not enough investigation, setting selection range into a non-editable node in an editing host doesn't cause stealing focus from its ancestor editing host. However, I feel this does make sense to me. When I click such node on Gecko, focus is set to the editor but selection is set to beginning of the editor. However, on the others, the editor is focused but user cannot modify anything until resetting focus with Tab key navigation.  So, I think that we should do same behavior in this case, we need to change our editor behavior first. So, it's out of scope of this bug.

The other is, Chromium cannot set selection range across editable node and non-editable node nor across focused editing host. If we should do same behavior in this case, we need to change our Selection API behavior first. So, this case is also really out of scope of this bug because this bug changes only focus behavior after using current our Selection API.

So, in this bug, I keep going with current design of my patches and I'll file two bugs for above issues.
(In reply to Olli Pettay [:smaug] from comment #17)
> ::: layout/generic/test/test_moving_focus_by_selection_API.html:1
> (Diff revision 1)
> > +<!DOCTYPE>
> 
> Making this a wpt test would be really nice, even if it was still in
> testing/web-platform/mozilla/tests .

Although, I start to rewrite the tests, I cannot run the test with |mach| due to "EnvironmentError: http server on port 8000 failed to start". I'll try to fix this, but if I cannot find the way in a couple of days, I'll continue to use mochitest for this bug (anyway, I need a couple of days to rewrite all tests).
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review118182

A couple of drive-by nits:

::: layout/generic/Selection.h:358
(Diff revision 1)
> +  nsPIDOMWindowOuter* GetWindow() const;
> +  nsIHTMLEditor* GetHTMLEditor() const;
> +
> +  /**
> +   * GetCommonEditingHost() returns common editing host of all ranges if there
> +   * is.  Otherwise, e.g., one of the ranges is in non-editable element or

Looks like there is something missing after the "or" ?

::: layout/generic/nsSelection.cpp:6313
(Diff revision 1)
> +    auto& range = rangeData.mRange;
> +    if (NS_WARN_IF(!range)) {
> +      continue;
> +    }

I'm pretty sure mRange can never be null, so I don't think we should add a null-check here since other code in this file don't do that.  Having a null-check creates the impression that it might be null, so it's slightly misleading when it can't.

Also, I don't think we need the extra indirection we get from using "auto& range" (which is a RefPtr<nsRange>&), we can just use nsRange* instead and dereference the RefPtr once.  (Not particularly important here since it's only used once after removing the null-check, but it's for being consistent with other code in this file.)
(In reply to Mats Palmgren (:mats) from comment #22)

> Also, I don't think we need the extra indirection we get from using "auto&
> range" (which is a RefPtr<nsRange>&), we can just use nsRange* instead and
> dereference the RefPtr once.  (Not particularly important here since it's
> only used once after removing the null-check, but it's for being consistent
> with other code in this file.)
Good point. Yet another example where auto is bad. auto is so very very rarely a useful thing. 
Usually it just makes code harder to read and more error prone.
(exception being some iterations where iteration type is complicated.)
How do the changes coming for bug 1343037 affect this bug?
Flags: needinfo?(masayuki)
(In reply to Jet Villegas (:jet) from comment #24)
> How do the changes coming for bug 1343037 affect this bug?

Perhaps, no. The bug changes around <input>/<textarea> but this bug about for contenteditable.
Flags: needinfo?(masayuki)
(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #18)
> ::: dom/base/nsRange.h:348
> (Diff revision 1)
> >  
> > +  class MOZ_RAII AutoCalledFromExternalSetter final
> > +  {
> > +  private:
> > +    RefPtr<nsRange> mRange;
> > +    bool mOldValue;
> 
> and this looks rather simple... couldn't you just use AutoRestore like what
> you do with Selection.

Unfortunately, we cannot use AutoRestor in Range due to a bit field... I'll just avoid grabbing the instance with RefPtr.
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review117974

> This is possibly rather hot code, at least in some microbenchmarks. Could GetHTMLEditor at least return already_AddRefed<nsIEditor> ?

I'll create nsIEditor* GetEditor() and checks it after other checks.  However, looks like that we need additional optimization for nsContentUtils::GetHTMLEditor(). I think that we should do that in follow up bug because it could be used in a lot of places.
Comment on attachment 8839896 [details]
Bug 1318312 part.1 Add automated tests for checking focus move at using Selection API

https://reviewboard.mozilla.org/r/114482/#review117950

> Making this a wpt test would be really nice, even if it was still in testing/web-platform/mozilla/tests .

Rewritten as web-platform-tests. I'll request review again.

> Some comment why P and A are special cased here would make reading this easier.

New test doesn't include such "intelligent" code.

> Call this just collapseRange? or collapseSelectionRange ?

New test is separated to multiple files, so, there are more simple helper functions.
Comment on attachment 8839897 [details]
Bug 1318312 part.2 Mark Selection as "called by JS" when every Selection API which may cause changing selection is called by JS

https://reviewboard.mozilla.org/r/114484/#review119638

::: dom/base/nsRange.h:347
(Diff revision 1)
>                                     size_t aRangeEnd);
>  
> +  class MOZ_RAII AutoCalledFromExternalSetter final
> +  {
> +  private:
> +    RefPtr<nsRange> mRange;

Okay, we can assume that *JS() is called with grabbing the instance. However, we still need this class because cannot use AutoRestor for bit field member. So, this will be just a reference.
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review117974

> Do other browsers change focus synchronously when range or selection APIs are used? Worth to test.

Yeah, Chrome fires focus event synchronously.
https://jsfiddle.net/d_toybox/ut07rfhr/

> * click
> * focus
> * addRange()ed
(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #19)
> ::: layout/generic/Selection.h:385
> (Diff revision 1)
> > +   *      <div id="c" contenteditable>
> > +   *        de[sc]endant
> > +   *  in this case, this returns nullptr because the second range is in
> > +   *  non-editable area.
> > +   */
> > +  Element* GetCommonEditingHost();
> 
> Since this method has rather unexpected behavior when there are ranges in
> non-editable areas, could we call this something like
> GetCommonEditingHostForAllRanges()
> 
> And what happens if there is a selection range outside top level content
> editable? I assume this returns null? Want to add a comment about it.

I assume that Selection never has ranges in different document (except caused by bug). Do I need to what kind of check exactly? (If there is no editable content, it returns nullptr because of common ancestor is non-editable.)
Flags: needinfo?(bugs)
I'm waiting to open smaug's review queue ;-)
(In reply to Masayuki Nakano [:masayuki] from comment #32)

> I assume that Selection never has ranges in different document (except
> caused by bug). Do I need to what kind of check exactly?
No

 (If there is no
> editable content, it returns nullptr because of common ancestor is
> non-editable.)

right, and I was just hoping to see a comment about it
Flags: needinfo?(bugs)
Attachment #8839896 - Flags: review?(bugs)
Attachment #8839897 - Flags: review?(bugs)
Attachment #8839898 - Flags: review?(bugs)
Comment on attachment 8839896 [details]
Bug 1318312 part.1 Add automated tests for checking focus move at using Selection API

https://reviewboard.mozilla.org/r/114482/#review120708

rs+ for this. Great to see tests in wpt. Hopefully we'll get some spec for this stuff and then all the browsers can reuse these tests.
Attachment #8839896 - Flags: review?(bugs) → review+
Comment on attachment 8839897 [details]
Bug 1318312 part.2 Mark Selection as "called by JS" when every Selection API which may cause changing selection is called by JS

https://reviewboard.mozilla.org/r/114484/#review120710

r+, assuming the next patch really needs the flags.

::: dom/base/nsRange.h:345
(Diff revision 2)
>                                     const nsTArray<const nsRange*>& aRanges,
>                                     size_t aRangeStart,
>                                     size_t aRangeEnd);
>  
> +  // Assume that this is guaranteed that this is held by the caller when
> +  // this is used.  (Note that we cannot use AutoRestor for mCalledByJS

nit, AutoRestore

::: dom/base/nsRange.h:356
(Diff revision 2)
> +    bool mOldValue;
> +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +
> +  public:
> +    explicit AutoCalledByJSSetter(nsRange& aRange
> +                                    MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

indentation

::: layout/generic/Selection.h:171
(Diff revision 2)
>    bool IsCollapsed() const;
> -  void Collapse(nsINode& aNode, uint32_t aOffset, mozilla::ErrorResult& aRv);
> -  void CollapseToStart(mozilla::ErrorResult& aRv);
> -  void CollapseToEnd(mozilla::ErrorResult& aRv);
>  
> -  void Extend(nsINode& aNode, uint32_t aOffset, mozilla::ErrorResult& aRv);
> +  // *JS() methods are mapped to Selection.*() of DOM.

I would drop "of DOM"

::: layout/generic/Selection.h:173
(Diff revision 2)
> -  void CollapseToStart(mozilla::ErrorResult& aRv);
> -  void CollapseToEnd(mozilla::ErrorResult& aRv);
>  
> -  void Extend(nsINode& aNode, uint32_t aOffset, mozilla::ErrorResult& aRv);
> +  // *JS() methods are mapped to Selection.*() of DOM.
> +  // They may move focus only when the range represents normal selection.
> +  // These methods shouldn't be used from internal.

internal what?

::: layout/generic/Selection.h:247
(Diff revision 2)
>  
>    void ScrollIntoView(int16_t aRegion, bool aIsSynchronous,
>                        int16_t aVPercent, int16_t aHPercent,
>                        mozilla::ErrorResult& aRv);
>  
> +  // Following methods should be used for internal use instead of *JS().

This comment is also a bit unclear.
Perhaps
Non-JS callers should use the following methods:
Attachment #8839897 - Flags: review?(bugs) → review+
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review120712

::: layout/generic/nsSelection.cpp:6391
(Diff revision 2)
> +        GetEditor()) {
> +      RefPtr<Element> newEditingHost = GetCommonEditingHostForAllRanges();
> +      // When all selected ranges are in an editing host, it should take focus.
> +      if (newEditingHost) {
> +        MOZ_ASSERT(!newEditingHost->IsInNativeAnonymousSubtree());
> +        IgnoredErrorResult erv;

erv? maybe err. Same also elsewhere

::: layout/generic/nsSelection.cpp:6403
(Diff revision 2)
> +        nsFocusManager* fm = nsFocusManager::GetFocusManager();
> +        nsCOMPtr<nsPIDOMWindowOuter> focusedWindow;
> +        nsIContent* focusedContent =
> +          fm->GetFocusedDescendant(window, false,
> +                                   getter_AddRefs(focusedWindow));
> +        nsCOMPtr<Element> focusedElement = do_QueryInterface(focusedContent);

Did you test this also in case focused element is in iframe?

::: layout/generic/nsSelection.cpp:6414
(Diff revision 2)
> +                               "Failed to blur focused element");
> +        }
> +      }
> +    }
> +  }
> +

Nothing seems to guarantee Selection object stays alive after dispatching the events.
At least nsRange should have _local_ strong variable (RefPtr) keeping Selection object alive.
But please check all the callers of this method, and also the version of the method which takes boolean param.

Or, do we need to dispatch focus/blur before the rest of the stuff the method does? If so, add a comment that NotifySelectionChanged must be called after focus/blur. If not, move focus/blur to happen after NotifySelectionChanged (and if this is the case, there wouldn't be need for strong variables.).
Attachment #8839898 - Flags: review?(bugs) → review-
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review120712

> erv? maybe err. Same also elsewhere

Hmm, I want common name for it like |rv|...

> Did you test this also in case focused element is in iframe?

Oh, good point! I'll add wpt test for it.

> Nothing seems to guarantee Selection object stays alive after dispatching the events.
> At least nsRange should have _local_ strong variable (RefPtr) keeping Selection object alive.
> But please check all the callers of this method, and also the version of the method which takes boolean param.
> 
> Or, do we need to dispatch focus/blur before the rest of the stuff the method does? If so, add a comment that NotifySelectionChanged must be called after focus/blur. If not, move focus/blur to happen after NotifySelectionChanged (and if this is the case, there wouldn't be need for strong variables.).

> do we need to dispatch focus/blur before the rest of the stuff the method does?

I guess so because if selection listener needs to touch something in new active editing host, the editor should be initialized before that.
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review120712

> Oh, good point! I'll add wpt test for it.

Chrome doesn't move focus to any editing host in other window and active the window.  However, it doesn't make sense when focus is moved to the window.

So, I think that we should activate editing host in other window but shouldn't move focus of current window.

> > do we need to dispatch focus/blur before the rest of the stuff the method does?
> 
> I guess so because if selection listener needs to touch something in new active editing host, the editor should be initialized before that.

Okay, grabbing the Selection instance at calling NotifySelectionListeners() at nsRange::DoSetRange() and nsFrameSelection::NotifySelectionListeners().
(In reply to Masayuki Nakano [:masayuki] from comment #49)
> Please check the new test for iframe case:
> https://reviewboard.mozilla.org/r/114482/diff/3#index_header

Oops, this link isn't a link to the specific file (sigh... MozReview staff...). Please check |Selection_addRange_into_iframe.html| in the review.
Comment on attachment 8839898 [details]
Bug 1318312 part.3 Selection should move focus at every selection change when it's called by JS

https://reviewboard.mozilla.org/r/114486/#review121602

::: dom/base/nsRange.cpp:971
(Diff revision 3)
>    mRoot = aRoot;
>  
>    // Notify any selection listeners. This has to occur last because otherwise the world
>    // could be observed by a selection listener while the range was in an invalid state.
>    if (mSelection) {
> -    mSelection->NotifySelectionListeners(mCalledByJS);
> +    // Be aware, this range may be modified or not a range for selection after

... or stop being a range for selection after...
Attachment #8839898 - Flags: review?(bugs) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cd7fe0526619
part.1 Add automated tests for checking focus move at using Selection API r=smaug
https://hg.mozilla.org/integration/autoland/rev/7d9c30109c50
part.2 Mark Selection as "called by JS" when every Selection API which may cause changing selection is called by JS r=smaug
https://hg.mozilla.org/integration/autoland/rev/22e1a32de5b8
part.3 Selection should move focus at every selection change when it's called by JS r=smaug
https://hg.mozilla.org/mozilla-central/rev/cd7fe0526619
https://hg.mozilla.org/mozilla-central/rev/7d9c30109c50
https://hg.mozilla.org/mozilla-central/rev/22e1a32de5b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
For writers of MDN:

Now, following API may change focus to or from editing host, but not move focus to nor from other focus able elements:

Selection.collapse()
Selection.collapseToStart()
Selection.collapseToEnd()
Selection.extend()
Selection.selectAllChildren()
Selection.addRange()
Selection.setBaseAndExtent()

# Only when the range is retrieved by Selection.getRangeAt().
Range.setStart()
Range.setEnd()
Range.setStartBefore()
Range.setStartAfter()
Range.setEndBefore()
Range.setEndAfter()
Range.collapse()
Range.selectNode()
Range.selectNodeContents()


When these methods are called, Gecko looks for a common ancestor of *all* selected ranges.  If all selected ranges' common ancestors are editable and the all common ancestors are in an editing host, the editing host will be get focus.  Otherwise, when an editing host has focus but some selected range becomes outside of it or into non-editable node in the editing host itself, the editing host will lose focus.

If you want to confirm something about the behavior, please feel free to ask me with ni?.
Depends on: 1347809
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #58)
> For writers of MDN:
> 
> Now, following API may change focus to or from editing host, but not move
> focus to nor from other focus able elements:
> 
> Selection.collapse()
> Selection.collapseToStart()
> Selection.collapseToEnd()
> Selection.extend()
> Selection.selectAllChildren()
> Selection.addRange()
> Selection.setBaseAndExtent()
> 
> # Only when the range is retrieved by Selection.getRangeAt().
> Range.setStart()
> Range.setEnd()
> Range.setStartBefore()
> Range.setStartAfter()
> Range.setEndBefore()
> Range.setEndAfter()
> Range.collapse()
> Range.selectNode()
> Range.selectNodeContents()
> 
> 
> When these methods are called, Gecko looks for a common ancestor of *all*
> selected ranges.  If all selected ranges' common ancestors are editable and
> the all common ancestors are in an editing host, the editing host will be
> get focus.  Otherwise, when an editing host has focus but some selected
> range becomes outside of it or into non-editable node in the editing host
> itself, the editing host will lose focus.
> 
> If you want to confirm something about the behavior, please feel free to ask
> me with ni?.

Thanks for the information Masayuki!

I have tried to document this, but I don't think I understand it properly. Can you look at my description here and let me know if this is accurate?

https://developer.mozilla.org/en-US/docs/Web/API/Selection#Behavior_of_Selection_API_in_terms_of_editable_ancestor_focus

I have also added a note to the Fx55 developer notes, which could do with a tech review:

https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

I also think we should add a definition of "Editing host" to the document, as I can't really find one that makes any sense. Can you give me a definition to work with?

Many thanks.
Flags: needinfo?(masayuki)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #59)
> I have tried to document this, but I don't think I understand it properly.
> Can you look at my description here and let me know if this is accurate?
> 
> https://developer.mozilla.org/en-US/docs/Web/API/
> Selection#Behavior_of_Selection_API_in_terms_of_editable_ancestor_focus

Thank you for your work, and sorry for the delay.

> The selection API has specific rules that...

Although, I'm not sure the exact meaning of "rules" here, if you meant that they were declared by Selection API or other web standard spec, it'd be wrong. We just take similar behavior with other browsers (not exactly same in some cases).

> when part or all of a selection is inside an editable ancestor (i.e. an
> element containing text with contenteditable set on it). These rules are
> true for selections made using the following methods:

Hmm, we'll change focus behavior after the methods-calls, only when:

* an editing host had focus but new selection range is outside of it.
* selection was outside of an editing host but new range is in an editing host.

So, we'll start to move focus with the methods only when we hit one of the conditions. I guess that it's easier to understand if you explain the fact that the methods may move focus but that is limited to set focus to an editing host or steal focus from an editing host (i.e., not set focus to the other elements like <a href=...> nor not steal focus from the other elements except setting focus to an editing host).

> when part or all of a selection is inside an editable ancestor (i.e. an
> element containing text with contenteditable set on it). These rules are
> true for selections made using the following methods:

Perhaps, you should use the term "editing host" for making the meaning clearer and need to change the last half (focus may be moved from non-editing host to an editing host. i.e., focus won't be moved to non-editing host nor work as .blur()).

> I have also added a note to the Fx55 developer notes, which could do with a
> tech review:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

So, using "editing host" is better for this too.

> I also think we should add a definition of "Editing host" to the document,
> as I can't really find one that makes any sense. Can you give me a
> definition to work with?

"editing host" is declared here: https://w3c.github.io/editing/execCommand.html#editing-host
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #60)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #59)
> > I have tried to document this, but I don't think I understand it properly.
> > Can you look at my description here and let me know if this is accurate?
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/
> > Selection#Behavior_of_Selection_API_in_terms_of_editable_ancestor_focus
> 
> Thank you for your work, and sorry for the delay.

No worries! Thank you so much for the help with trying to figure this out. I think we are a lot closer now. I've updated the main description according to your comments:

https://developer.mozilla.org/en-US/docs/Web/API/Selection#Behavior_of_Selection_API_in_terms_of_editing_host_focus_changes

Added a editing host definition to the glossary:

https://developer.mozilla.org/en-US/docs/Web/API/Selection#Glossary

And updated the rel note:

https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Can I trouble you to give it another look?
Flags: needinfo?(masayuki)
Depends on: 1367460
Hi, cmills. Sorry for the delay to reply.

I have a bad news for you. Due to bug 1367460, I partially backed out this bug's change.

The change is, Selection API won't steal focus from focused editing host even if Selection API moves selection to an non-editable node nor outside of it. This is better compatible behavior with Chromium. So, could you remove this behavior from the documents you wrote?

So, you need to modify around:

"The behaviours are:" of https://developer.mozilla.org/en-US/docs/Web/API/Selection#Behavior_of_Selection_API_in_terms_of_editing_host_focus_changes

and

drop "or outside" of https://developer.mozilla.org/en-US/Firefox/Releases/55#Selection_API

Otherwise, looks fine to me.
Flags: needinfo?(masayuki) → needinfo?(cmills)
No problem. I've updated the docs again. Can you please give one final check to make sure I've got this right?

Thanks!
Flags: needinfo?(cmills) → needinfo?(masayuki)
Thanks! Looks fine to me. Thank you for your work!
Flags: needinfo?(masayuki)
Depends on: 1377752
Depends on: 1379561
Attached file testcase.html — 
It turns out that this may not be fully web-compatible. For instance, Chrome and Safari do not scroll up to the element in the attached testcase with range-collapsing, while Edge (and now Firefox) do.

In a nutshell, this new behavior broke the browser sniffing for the Rangy library [1], which is used by WSYIHTML [2], in turn causing community.spiceworks.com's pages [3] to scroll back up after the user scrolls down to the bottom of the page with the end key.

Rangy's browsing sniffing code can be improved to compensate (they hack around the Edge case instead of a proper fix), but based on the comments in Spicework's code, WSYIHTML broke API compatibility and they're still stuck on an older version, so I worry that there could be more sites affected by this change.

[1] https://github.com/timdown/rangy/issues/292
[2] https://github.com/Voog/wysihtml
[3] https://github.com/webcompat/web-bugs/issues/8400
(In reply to Thomas Wisniewski from comment #66)
> Created attachment 8889672 [details]
> testcase.html
> 
> It turns out that this may not be fully web-compatible. For instance, Chrome
> and Safari do not scroll up to the element in the attached testcase with
> range-collapsing, while Edge (and now Firefox) do.
> 
> In a nutshell, this new behavior broke the browser sniffing for the Rangy
> library [1], which is used by WSYIHTML [2], in turn causing
> community.spiceworks.com's pages [3] to scroll back up after the user
> scrolls down to the bottom of the page with the end key.

Thomas, please file a new bug.
Depends on: 1436906
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: