Closed
Bug 1375825
Opened 7 years ago
Closed 7 years ago
ContentEventHandler::ExpandToClusterBoundary() ignores result of nsIFrame::PeekOffsetCharacter()
Categories
(Core :: DOM: UI Events & Focus Handling, enhancement)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(2 files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=881877afda67a83b9db01047b87607ac2ecac206
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years 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•7 years 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.
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea3ce53b49d1e3b0ba09cd98ab7ecab998a0611
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years 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?
Assignee | ||
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aaedcdf7003a06e73560432d430c3aebbb4bb31
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e02848932cd https://hg.mozilla.org/mozilla-central/rev/d3ba6f330ec1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jfkthame)
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•