ContentEventHandler::ExpandToClusterBoundary() ignores result of nsIFrame::PeekOffsetCharacter()

RESOLVED FIXED in Firefox 56

Status

()

Core
Event Handling
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

https://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/dom/events/ContentEventHandler.cpp#964-966

Ignoring the result of nsIFrame::PeekOffsetCharacter() causes ContentEventHandler::ExpandToClusterBoundary() returning unexpected value. E.g., given 6 with true for aForward, the result becomes 5.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

8 months ago
mozreview-review
Comment on attachment 8880857 [details]
Bug 1375825 - part1: Create test for querying text rect in a non-editable element in a contenteditable element

https://reviewboard.mozilla.org/r/152206/#review157492

rs+, though the commit message should say something about the change to nsQueryContentEventResult::GetText
Attachment #8880857 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

8 months ago
mozreview-review
Comment on attachment 8880858 [details]
Bug 1375825 - part2: ContentEventHandler::ExpandToClusterBoundary() should check the return value of nsTextFrame::PeekOffsetCharacter()

https://reviewboard.mozilla.org/r/152208/#review157658

::: layout/generic/nsIFrame.h:4129
(Diff revision 2)
>     *         see enum FrameSearchResult for more details.
>     */
> -  virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
> -                                     bool aRespectClusters = true) = 0;
> +  virtual FrameSearchResult PeekOffsetCharacter(
> +                              bool aForward, int32_t* aOffset,
> +                              bool aRespectClusters = true,
> +                              bool aIgnoreUserStyleAll = false) = 0;

Could we please replace the bool parameters here with a single flags enum instead? At least for the two trailing options; maybe aForward is easier kept separate.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

8 months ago
mozreview-review
Comment on attachment 8880858 [details]
Bug 1375825 - part2: ContentEventHandler::ExpandToClusterBoundary() should check the return value of nsTextFrame::PeekOffsetCharacter()

https://reviewboard.mozilla.org/r/152208/#review157768

::: layout/generic/nsIFrame.h:4144
(Diff revision 3)
> +                              const PeekOffsetCharacterOptions& aOptions =
> +                                PeekOffsetCharacterOptions()) = 0;

As PeekOffsetCharacterOptions is only 2 bytes, it's probably cheaper to pass it by value than by reference, isn't it?
(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Comment on attachment 8880858 [details]
> Bug 1375825 - part2: ContentEventHandler::ExpandToClusterBoundary() should
> check the return value of nsTextFrame::PeekOffsetCharacter()
> 
> https://reviewboard.mozilla.org/r/152208/#review157768
> 
> ::: layout/generic/nsIFrame.h:4144
> (Diff revision 3)
> > +                              const PeekOffsetCharacterOptions& aOptions =
> > +                                PeekOffsetCharacterOptions()) = 0;
> 
> As PeekOffsetCharacterOptions is only 2 bytes, it's probably cheaper to pass
> it by value than by reference, isn't it?

Ah, yes. But is it meaningful change? AFAIK, C++ wants to use the fastest memory block size and align objects for it. I think that it's always same as the pointer size (at least platforms we support).

I'll update the patch, but I'm not 100% sure it's good thing.
Flags: needinfo?(jfkthame)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8880858 [details]
Bug 1375825 - part2: ContentEventHandler::ExpandToClusterBoundary() should check the return value of nsTextFrame::PeekOffsetCharacter()

https://reviewboard.mozilla.org/r/152208/#review158012

LGTM, thanks.

::: layout/generic/nsIFrame.h:4142
(Diff revision 4)
> -  virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset,
> -                                     bool aRespectClusters = true) = 0;
> +  virtual FrameSearchResult PeekOffsetCharacter(
> +                              bool aForward, int32_t* aOffset,
> +                              PeekOffsetCharacterOptions aOptions =
> +                                PeekOffsetCharacterOptions()) = 0;

Here and throughout the patch, my preference would be to put the method name on a new line, rather than line-break after the open-paren:

    virtual FrameSearchResult
    PeekOffsetCharacter(bool aForward, ...)

if you don't mind making that update.
Attachment #8880858 - Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

8 months ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2e02848932cd
part1: Create test for querying text rect in a non-editable element in a contenteditable element r=smaug
https://hg.mozilla.org/integration/autoland/rev/d3ba6f330ec1
part2: ContentEventHandler::ExpandToClusterBoundary() should check the return value of nsTextFrame::PeekOffsetCharacter() r=jfkthame

Comment 20

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e02848932cd
https://hg.mozilla.org/mozilla-central/rev/d3ba6f330ec1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

7 months ago
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.