1616257 - Redesign `WSRunScanner::NextVisibleNode()` and `WSRunScanner::PriorVisibleNode()`

Status: RESOLVED FIXED

(Core :: DOM: Editor, task, P5)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Theses methods have a lot of out parameters and it's really unclear what they do. We should make them return a struct and the its member methods should explain what do each one means.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The coming patches add a lot of NS_ASSERTIONs which shouldn't occur. However, 2 of them are hit with some mochitests and wpts. I'll file follow up bugs for them later.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1616257 - part 1: Redesign `WSRunScanner::GetWSBoundingParent()` r=m_kato!

Its result has 3 meanings:

1. an editable block element for container of mScanStartPoint.
2. a topmost inline editable content for container of mScanStartPoint if there
   is no editable block parent.
3. container of mScanStartPoint if it's a block (either editable or
   non-editable).
4. container of mScanStartPoint if its parent is not editable and a inline
   content.

#1, #2 and editable case of #3 make sense because the results are topmost
editable content in current context. On the other hand, non-editable case
of #3 and #4 are caused by unexpected wrong fallback code.

So, let's make it always returns the content in the former meaning and if
the caller needs the latter one, they should use the container by themselves.
Therefore, for making what's the start of the search, this patch also makes
new method take start content instead of hiding mScanStartPoint from the
callers.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1616257 - part 2: Change `mStartReasonNode` and `mEndReasonNode` to `nsIContent` r=m_kato!

Now, all setter guarantee that they are subclass instances of nsIContent.

Depends on D63610

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1616257 - part 3: Make `WSRunScanner::NextVisibleNode()` and `WSRunScanner::PriorVisibleNode()` return stack only class instance which storing the visible node as `nsIContent` r=m_kato!

They are really messy because they take a lot of out parameters, and these
out parameter meaning is really unclear. Therefore, they should return
a stack only class instance which explain the meaning with getter methods.
And also it should store the result node as nsIContent.

And also this patch adds static methods for them for their users which don't
need WSRunScanner instance.

Depends on D63612

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1616257 - part 4: Use content of `WSScanResult` instead of reason content of `WSRunScanner` if they are same r=m_kato!

When WSScanResult::ReachedCurrentBlockBoundary() returns true, reached content
of backward scan result is same as the scanner's GetStartReasonContent() and
reached content of forward scan result is same as the scanner's
GetEndReasonContent(). For making code in the blocks of
if (foo.ReachedCurrentBlockBoundary()) easier to understand, we should use
the result's content.

Depends on D63613

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1616257 - part 5: Make `WSRunObject` stop keeping `HTMLEditor` as a friend class r=m_kato!

With the preceding patches, HTMLEditor mostly does not need to be a friend of
WSRunObject.

This patch stops it, and adding some self-documented methods for checking
mStartReason and mEndReason, and also adding self-documented alternative
methods of GetStartReasonContent() and GetEndReasonContent().

Depends on D63615

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8505320d216
part 1: Redesign `WSRunScanner::GetWSBoundingParent()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9b52ba2aa0da
part 2: Change `mStartReasonNode` and `mEndReasonNode` to `nsIContent` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1623be3a5824
part 3: Make `WSRunScanner::NextVisibleNode()` and `WSRunScanner::PriorVisibleNode()` return stack only class instance which storing the visible node as `nsIContent` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/32eb7d5a2880
part 4: Use content of `WSScanResult` instead of reason content of `WSRunScanner` if they are same r=m_kato
https://hg.mozilla.org/integration/autoland/rev/97385d484fbf
part 5: Make `WSRunObject` stop keeping `HTMLEditor` as a friend class r=m_kato

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f8505320d216
https://hg.mozilla.org/mozilla-central/rev/9b52ba2aa0da
https://hg.mozilla.org/mozilla-central/rev/1623be3a5824
https://hg.mozilla.org/mozilla-central/rev/32eb7d5a2880
https://hg.mozilla.org/mozilla-central/rev/97385d484fbf