Closed Bug 1110039 Opened 10 years ago Closed 10 years ago

Refactor Touch/SelectionCarets as AccessibleCaret

Categories

(Core :: DOM: Selection, defect, P4)

Product:
Core
Component:
DOM: Selection
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: mtseng, Assigned: TYLin)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(9 files, 26 obsolete files)

Part 1 - Add nsLayoutUtils::ClampRectToScrollFrames (v1)
(deleted), patch
roc
: review+
mtseng
: feedback+
Details | Diff | Splinter Review
Part 2.1 - Add logger facility (v2, carry roc r+)
(deleted), patch
TYLin
: review+
Details | Diff | Splinter Review
Part 2.5 - Add all files to build system (v2, carry roc r+)
(deleted), patch
TYLin
: review+
Details | Diff | Splinter Review
Part 4 - Hook new classes into the system (v2, carry roc r+)
(deleted), patch
TYLin
: review+
Details | Diff | Splinter Review
Part 2.3 - Add AccessibleCaretManager (v4)
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Part 2.4 - Add AccessibleCaretEventHub (v4)
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
Part 2.2 - Add AccessibleCaret (v6, carry roc r+)
(deleted), patch
TYLin
: review+
Details | Diff | Splinter Review
Part 5 - Reuse marionette tests for AccessibleCaret (v3)
(deleted), patch
mtseng
: review+
Details | Diff | Splinter Review
Part 3 - Add gtest for AccessibleCaretEventHub (v4, carry roc r+)
(deleted), patch
TYLin
: review+
Details | Diff | Splinter Review
Currently, we maintain logic of touch and selection carets in different files. But they have many common functions and we cannot easily add more logic which is related to those carets. Which means our carets are fragile. So we have a plan to refactor those carets to let them become more maintainable. We'll also add gtest for those codes. Current wip is at [1]. I'll upload to here once it is reviewable. [1]: https://github.com/mephisto41/gecko-dev/tree/copypaste-refactor
Priority: -- → P3
This bug will be landed at V3, set as p4 now.
Priority: P3 → P4
Blocks: AccessibleCaret
No longer blocks: CopyPasteLegacy
URL: https://github.com/mephisto41/gecko-d...
OS: Mac OS X → All
Hardware: x86 → All
ClampRectToScrollFrames generalizes IsRectVisibleInScrollFrames by returning the clamped rect in scroll frames. IsRectVisibleInScrollFrames could be implemented by checking whether the clamped rect is empty or not.
Attached patch Part 3 - Add gtest for CopyPasteEventHub (v1) (obsolete) (deleted) — — Splinter Review
Attachment #8592092 - Flags: feedback?(mtseng)
Attached patch Part 4 - Hook new classes into the system (v1) (obsolete) (deleted) — — Splinter Review
The necessary modifications are the same as SelectionCarets. For convenience, Touch/SelectionCarets will be disabled whenever AccessibleCaret preference is enabled.
Attachment #8592093 - Flags: feedback?(mtseng)
The new class should behave like the old TouchCaret and SelectionCarets. I simply refactor the setUp() to support both the old and new preferences. _test_handle_tilt_when_carets_overlap_to_each_other() is modified because AccessibleCaret does not inflate the caret hit rectangle as TouchCaret/SelectionCarets did. The point for tilt caret edges need to shrink a bit.
Attachment #8592094 - Flags: feedback?(mtseng)
Attachment #8592089 - Flags: feedback?(mtseng)
Attachment #8592089 - Attachment description: Part 1 - Add nsLayoutUtils::ClampRectToScrollFrames → Part 1 - Add nsLayoutUtils::ClampRectToScrollFrames (v1)
Attachment #8592091 - Attachment description: Part 2 - Unify TouchCaret and SelectionCarets → Part 2 - Unify TouchCaret and SelectionCarets (v1)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #4) > Created attachment 8592091 [details] [diff] [review] > Part 2 - Unify TouchCaret and SelectionCarets > > We added new classes CopyPasteEventHub, CopyPasteManager, and > AccessibleCaret to unify the logic in TouchCaret and SelectionCarets. > See each header file for the description about what the class does. > > Note: The logic to dispatch information to Gaia will be implemented in > a follow-up bug. > > Some notable technical difference between AccessibleCaret and the > Touch/SelectionCarets. > * The anonymous dom element containing a caret image will be created by > AccessibleCaret by using the API landed in bug 1020244 instead of > being created by nsCanvasFrame. > * CopyPasteManager uses two AccessibleCarets to unify the functionality > provided by TouchCaret and SelectionCarets. It has "cursor" mode and > "selection" mode, which corresponds to TouchCaret and SelectionCarets, > respectively. > * It's hard to exchange information about the status of event handling > between TouchCaret and SelectionCarets. Now CopyPasteEventHub is the > entry point for all events and callbacks. > * When developing SelectionCarets, we encounter performance issues > because SelectionCarets might dispatch unnecessary events to Gaia > driven by the selection changed events. SelectionCarets did not have > clear internal states to avoid that. To solve that, CopyPasteEventHub > implements state classes, and rely on the current states to call the > CopyPasteManager's handler only when it's needed. For example, when > dragging a caret, we do not interest in NotifySelectionChanged() for > updating the carets. Since we know a caret is being dragging, we can > call UpdateCarets() directly. Hence DragCaretState does override > OnSelectionChanged(). I suggest attach a state machine diagram for better understanding this patch.
Comment on attachment 8592091 [details] [diff] [review] Part 2 - Unify TouchCaret and SelectionCarets (v1) (In reply to Morris Tseng [:mtseng] from comment #8) > I suggest attach a state machine diagram for better understanding this patch. Will do. Will also break part 2 into smaller patches.
Attachment #8592091 - Attachment is obsolete: true
Attachment #8592091 - Flags: feedback?(mtseng)
Attached patch Part 2.1 - Add logger facility (v1) (obsolete) (deleted) — — Splinter Review
Attachment #8592700 - Flags: feedback?(mtseng)
Attached patch Part 2.2 - Add AccessibleCaret (v1) (obsolete) (deleted) — — Splinter Review
See AccessibleCaret.h for the class description. Technical difference between AccessibleCaret and Touch/SelectionCarets: The anonymous dom element containing a caret image will be created by AccessibleCaret by using the API landed in bug 1020244 instead of being created by nsCanvasFrame.
Attachment #8592701 - Flags: feedback?(mtseng)
Attached patch Part 2.3 - Add CopyPasteManager (v1) (obsolete) (deleted) — — Splinter Review
See CopyPasteManager.h for the class description. CopyPasteManager uses two AccessibleCarets to unify the functionality provided by TouchCaret and SelectionCarets. It has "cursor" mode and "selection" mode, which corresponds to TouchCaret and SelectionCarets, respectively.
Attachment #8592702 - Flags: feedback?(mtseng)
Attached patch Part 2.4 - Add CopyPasteEventHub (v1) (obsolete) (deleted) — — Splinter Review
See CopyPasteEventHub.h for the class description, and this link for the state transition diagram. https://wiki.mozilla.org/Copy_n_Paste#CopyPasteEventHub_State_Transition_Diagram Both TouchCaret and SelectionCarets have their event handling mechanism, which lead to a lot of code duplication. Now CopyPasteEventHub serves as the single entry point for all events and callbacks. We also encountered performance issues in SelectionCarets because many unnecessary events might be dispatched to Gaia driven by the selection changed events. SelectionCarets did not have clear internal states to avoid this. To solve it, CopyPasteEventHub implements state classes, and rely on the current states to call the CopyPasteManager's handler only when it's needed. For example, when dragging a caret, we do not interest in NotifySelectionChanged() for updating the carets. Since we've known a caret is being dragging, we can call UpdateCarets() directly. Hence DragCaretState does not override OnSelectionChanged().
Attachment #8592703 - Flags: feedback?(mtseng)
Attached patch Part 2.5 - Add all files to build system (v1) (obsolete) (deleted) — — Splinter Review
Attachment #8592704 - Flags: feedback?(mtseng)
Attachment #8592706 - Attachment is obsolete: true
Attachment #8592089 - Flags: feedback?(mtseng) → feedback+
Attachment #8592700 - Flags: feedback?(mtseng) → feedback+
Attachment #8592704 - Flags: feedback?(mtseng) → feedback+
Attachment #8592094 - Flags: feedback?(mtseng) → feedback+
Attachment #8592093 - Flags: feedback?(mtseng) → feedback+
Attachment #8592701 - Flags: feedback?(mtseng) → feedback+
Attachment #8592092 - Flags: feedback?(mtseng) → feedback+
Attachment #8592702 - Flags: feedback?(mtseng) → feedback+
Attachment #8592703 - Flags: feedback?(mtseng) → feedback+
Attachment #8592094 - Flags: review?(dburns)
Attachment #8592089 - Flags: review?(roc)
Attachment #8592092 - Flags: review?(roc)
Attachment #8592093 - Flags: review?(roc)
Attachment #8592700 - Flags: review?(roc)
Attachment #8592701 - Flags: review?(roc)
Attachment #8592702 - Flags: review?(roc)
Attachment #8592703 - Flags: review?(roc)
Attachment #8592704 - Flags: review?(roc)
Attachment #8592094 - Flags: review?(dburns) → review+
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Blocks: 1155493
Comment on attachment 8592701 [details] [diff] [review] Part 2.2 - Add AccessibleCaret (v1) Review of attachment 8592701 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/AccessibleCaret.h @@ +33,5 @@ > +// 1. Insert DOM Element as an anonymous conent which contains the caret image. > +// 2. Provide functions to change the caret appearance and position. > +// > +// All the rect or point used are relative to root frame except being specified > +// explicitly. This doesn't really explain what an AccessibleCaret *is*, i.e., what its associated state means. @@ +47,5 @@ > + Normal, > + NormalNotShown, > + Left, > + Right > + }; Document these values. @@ +49,5 @@ > + Left, > + Right > + }; > + bool IsVisuallyVisible() const; > + bool IsLogicallyVisible() const; Describe these functions and why they're different. @@ +52,5 @@ > + bool IsVisuallyVisible() const; > + bool IsLogicallyVisible() const; > + Appearance GetAppearance() const; > + void SetAppearance(Appearance aAppearance); > + void SetBarEnabled(bool aEnabled); Document this
Attachment #8592701 - Flags: review?(roc) → review-
Comment on attachment 8592702 [details] [diff] [review] Part 2.3 - Add CopyPasteManager (v1) Review of attachment 8592702 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/CopyPasteManager.h @@ +32,5 @@ > +class CopyPasteEventHub; > + > +// ----------------------------------------------------------------------------- > +// CopyPasteManager handles events and callbacks from CopyPasteEventHub, and > +// perform the real work to manipulate selection object and AccessibleCaret. Explain how many of these there are (one per PresShell?) @@ +75,5 @@ > + bool ChangeFocus(nsIFrame* aFrame) const; > + nsresult SelectWord(nsIFrame* aFrame, const nsPoint& aPoint) const; > + void SetSelectionDragState(bool aState) const; > + void SetSelectionDirection(nsDirection aDir) const; > + nsIFrame* FindFirstNodeWithFrame(bool aBackward, int32_t* aOutOffset) const; Document this method @@ +98,5 @@ > + // Member variables > + nscoord mOffsetYToCaretLogicalPosition = NS_UNCONSTRAINEDSIZE; > + nsIPresShell* mPresShell = nullptr; > + UniquePtr<AccessibleCaret> mFirstCaret; > + UniquePtr<AccessibleCaret> mSecondCaret; Explain what "first" and "second" mean here
Attachment #8592702 - Flags: review?(roc) → review-
Comment on attachment 8592703 [details] [diff] [review] Part 2.4 - Add CopyPasteEventHub (v1) Review of attachment 8592703 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/CopyPasteEventHub.cpp @@ +355,5 @@ > + aContext->SetState(aContext->NoActionState()); > + > + // We should always consume the event since we've pressed on the caret. > + return nsEventStatus_eConsumeNoDefault; > +} Easier to read if you move these method definitions up into the class declaration. ::: layout/base/CopyPasteEventHub.h @@ +38,5 @@ > +// concrete events. CopyPasteEventHub also synthesizes fake events such as > +// long-tap or scroll-end if APZ is not in use. > +// > +// See this link for state transition diagram on Mozilla wiki. > +// https://wiki.mozilla.org/Copy_n_Paste#CopyPasteEventHub_State_Transition_Diagram Good diagram, but I'd prefer it to be in the tree, either as ASCII art here or an image checked in and referenced here. Can you explain why CopyPasteEventHub is separate from CopyPasteManager? Are these really the right names? Would it make more sense to call these TouchCaretManager or something like that? These aren't involved in regular copy and paste so these names might confuse people. @@ +147,5 @@ > + > +// ----------------------------------------------------------------------------- > +// Base class for state. A concrete state should inherit this class, and > +// override the methods for handling the events or callbacks. A concrete state > +// is also responsible for transforming to the next concrete state. Can you explain why you chose this approach instead of, say, having a simple state enum value, where the On... methods are on CopyPasteEventHub and switch on the current state? Maybe that would be simpler than having separate state classes with virtual methods?
Attachment #8592703 - Flags: review?(roc) → review-
Re comment 18: One quick question about class naming. You're right that CopyPasteEventHub and CopyPasteManager might confuse people. How about call them AccessibleCaretEventHub and AccessibleCaretManager? CaretEventHub and CaretManager would be shorter, but people might think they're related to nsCaret. Which ones do you prefer?
Flags: needinfo?(roc)
Re comment 18: Q: Why CopyPasteEventHub is separate from CopyPasteManager? A: We'd like the state transitions in CopyPasteEventHub to be testable by gtest, so we put operations involving AcceesibleCaret, PresShell, Selection, etc. in CopyPasteManager. In this way, we can mock CopyPasteManager, and test the correctness of the state transition given various events, callbacks, and the returns values of mocked methods of CopyPasteManager. Q: Why implementing state classes with virtual methods rather than simple enum and switch case? A: When the idea of state transition emerged, I indeed modelled them by using enum and switch case. From the SelectionCarets experience, we already know all the events and callbacks need to be handled. But the states required to model the system were not so clearly understood. Then I found it's hard to add a new state into the system. The handling functions become large, the size of nested switch case grow, and some common operation are hard to extract. Then I start from scratch and try the states class design. I feel it's easier to add new state and focused on choosing which events or callbacks to handle by overriding them in a specific state. State::Enter() is convenient that I can put common code in it when multiple events lead the transition into this state. So is State::Leave(). All those State::OnXXX methods should be short and simple. And I hopes they're easier to read than a large nested switch case :-)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #19) > One quick question about class naming. You're right that CopyPasteEventHub > and CopyPasteManager might confuse people. How about call them > AccessibleCaretEventHub and AccessibleCaretManager? CaretEventHub and > CaretManager would be shorter, but people might think they're related to > nsCaret. Which ones do you prefer? AccessibleCaretEventHub/AccessibleCaretManager sounds good.
Flags: needinfo?(roc)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #20) > Q: Why CopyPasteEventHub is separate from CopyPasteManager? > A: We'd like the state transitions in CopyPasteEventHub to be testable by > gtest, so we put operations involving AcceesibleCaret, PresShell, Selection, > etc. in CopyPasteManager. In this way, we can mock CopyPasteManager, and > test the correctness of the state transition given various events, > callbacks, and the returns values of mocked methods of CopyPasteManager. OK. Please document that in the code and make sure it's clear to maintainers which functionality belongs in which class. > Q: Why implementing state classes with virtual methods rather than simple > enum and switch case? > A: When the idea of state transition emerged, I indeed modelled them by > using enum and switch case. From the SelectionCarets experience, we already > know all the events and callbacks need to be handled. But the states > required to model the system were not so clearly understood. Then I found > it's hard to add a new state into the system. The handling functions become > large, the size of nested switch case grow, and some common operation are > hard to extract. > > Then I start from scratch and try the states class design. I feel it's > easier to add new state and focused on choosing which events or callbacks to > handle by overriding them in a specific state. State::Enter() is convenient > that I can put common code in it when multiple events lead the transition > into this state. So is State::Leave(). All those State::OnXXX methods should > be short and simple. And I hopes they're easier to read than a large nested > switch case :-) OK.
Rename CopyPaste -> AccessibleCaret
Attachment #8592700 - Attachment is obsolete: true
Attachment #8597129 - Flags: review+
Attached patch Part 2.2 - Add AccessibleCaret (v2) (obsolete) (deleted) — — Splinter Review
Document enum Appearance and other methods suggested in comment 16.
Attachment #8592701 - Attachment is obsolete: true
Attachment #8597131 - Flags: review?(roc)
Attached patch Part 2.3 - Add AccessibleCaretManager (obsolete) (deleted) — — Splinter Review
See AccessibleCaretManager.h for the class description. AccessibleCaretManager uses two AccessibleCarets to unify the functionality provided by TouchCaret and SelectionCarets. It has "cursor" mode and "selection" mode, which corresponds to TouchCaret and SelectionCarets, respectively.
Attached patch Part 2.4 - Add AccessibleCaretEventHub (v2) (obsolete) (deleted) — — Splinter Review
Addressed comment 18. * Rename CopyPasteEventHub -> AccessibleCaretEventHub * Move State method definitions into class declaration * Check-in image and source code of the state transition diagram into layout/base/doc * Improve documentation of AccessibleCaretEventHub
Attachment #8592703 - Attachment is obsolete: true
Attachment #8597139 - Flags: review?(roc)
Rename CopyPaste -> AccessibleCaret
Attachment #8592704 - Attachment is obsolete: true
Attachment #8597143 - Flags: review+
* Rename TestCopyPasteEventHub -> TestAccessibleCaretEventHub * Style clean-up
Attachment #8592092 - Attachment is obsolete: true
Attachment #8597144 - Flags: review+
Rename CopyPaste -> AccessibleCaret
Attachment #8592093 - Attachment is obsolete: true
Attachment #8597147 - Flags: review+
Attached patch Part 2.3 - Add AccessibleCaretManager (v2) (obsolete) (deleted) — — Splinter Review
Addressed comment 17. * Rewrite documentation of AccessibleCaretManager * Document FindFirstNodeWithFrame, mFirstCaret, mSecondCaret, and other methods.
Attachment #8592702 - Attachment is obsolete: true
Attachment #8597136 - Attachment is obsolete: true
Attachment #8597152 - Flags: review?(roc)
Attached patch Part 2.2 - Add AccessibleCaret (v3) (obsolete) (deleted) — — Splinter Review
Remove the gray background color of AccessibleCaret for debugging in ua.css.
Attachment #8597131 - Attachment is obsolete: true
Attachment #8597131 - Flags: review?(roc)
Attachment #8597772 - Flags: review?(roc)
Attached patch Part 2.2 - Add AccessibleCaret (v4) (obsolete) (deleted) — — Splinter Review
Diff between v1 and v4 * Document enum Appearance and other methods suggested in comment 16. * Remove the gray background color of AccessibleCaret for debugging in ua.css. * Rename some functions.
Attachment #8597772 - Attachment is obsolete: true
Attachment #8597772 - Flags: review?(roc)
Attachment #8597916 - Flags: review?(roc)
Attached patch Part 2.3 - Add AccessibleCaretManager (v3) (obsolete) (deleted) — — Splinter Review
Diff between v1 and v3 * Addressed comment 17 by rewrite documentation of AccessibleCaretManager. * Document FindFirstNodeWithFrame, mFirstCaret, mSecondCaret, and other methods. * Function name changes in AccessibleCaret.
Attachment #8597152 - Attachment is obsolete: true
Attachment #8597152 - Flags: review?(roc)
Attachment #8597921 - Flags: review?(roc)
When nsDisplayListBuilder doesn't build caret, we need to skip AccessibleCaret frame by checking that the content of the frame is in native anonymous subtree and has the "moz-accessiblecaret" class. Thank :mtseng for providing this approach.
Attachment #8598469 - Flags: review?(roc)
Ports the patch for Touch/SelectionCarets in bug 1021499.
Attachment #8598471 - Flags: review?(roc)
Comment on attachment 8597916 [details] [diff] [review] Part 2.2 - Add AccessibleCaret (v4) Review of attachment 8597916 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good. ::: layout/base/AccessibleCaret.cpp @@ +63,5 @@ > + > +AccessibleCaret::Appearance > +AccessibleCaret::GetAppearance() const > +{ > + return mAppearance; A lot of these short functions should be defined in the header file for conciseness. @@ +77,5 @@ > + ErrorResult rv; > + CaretElement()->ClassList()->Remove(AppearanceString(mAppearance), rv); > + MOZ_ASSERT(!rv.Failed(), "Remove old appearance failed!"); > + > + CaretElement()->ClassList()->Add(AppearanceString(aAppearance), rv); Instead of adding a "none" class, it would be slightly better to simply not have a class set at all. So you would only remove the old mAppearance if it's not None/NormalNotShown, and you would only add the new appearance if it's not None/NormalNotShown. @@ +142,5 @@ > + > +Element* > +AccessibleCaret::SelectionBarElement() const > +{ > + return CaretElement()->GetLastElementChild(); More functions that could just go in the header @@ +288,5 @@ > + > +void > +AccessibleCaret::SetCaretElementPosition(nsIFrame* aFrame, const nsRect& aRect) > +{ > + // Transform position so that it relatives to containerFrame. "So that it's relative to containerFrame" ::: layout/base/AccessibleCaret.h @@ +33,5 @@ > +// anonymous content containing the caret image. The caret appearance and > +// position can be controlled by SetAppearance() and SetPosition(). > +// > +// All the rect or point used are relative to root frame except being specified > +// explicitly. Document that none of the methods of this class flush layout or style (if that's true!). @@ +97,5 @@ > + // Does two AccessibleCarets overlap? > + bool Intersects(const AccessibleCaret& rhs) const; > + > + // Is the position within the caret's rect? > + bool Contains(const nsPoint& aPosition) const; Document what this point is relative to @@ +146,5 @@ > + > + // Member variables > + Appearance mAppearance = Appearance::None; > + bool mBarEnabled = false; > + nsIPresShell* mPresShell = nullptr; Is this a weak reference? How does it get cleared when the presshell goes away? Please document this. @@ +148,5 @@ > + Appearance mAppearance = Appearance::None; > + bool mBarEnabled = false; > + nsIPresShell* mPresShell = nullptr; > + nsRefPtr<dom::AnonymousContent> mCaretElementHolder; > + nsRect mImaginaryCaretRect; Document what this is relative to. @@ +152,5 @@ > + nsRect mImaginaryCaretRect; > + > + // A no-op touch-start listener which prevents APZ from panning when dragging > + // the caret. > + nsRefPtr<DummyTouchListener> mDummyTouchListener{new DummyTouchListener()}; Initializing these members at declaration is a new C++ feature. Might be better for now to keep doing these in the constructor like other classes do.
Attachment #8597916 - Flags: review?(roc) → review-
Comment on attachment 8597921 [details] [diff] [review] Part 2.3 - Add AccessibleCaretManager (v3) Review of attachment 8597921 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good. ::: layout/base/AccessibleCaretManager.cpp @@ +41,5 @@ > + : mPresShell(aPresShell) > +{ > + if (mPresShell) { > + mFirstCaret = MakeUnique<AccessibleCaret>(mPresShell); > + mSecondCaret = MakeUnique<AccessibleCaret>(mPresShell); Any reason to not just make mFirstCaret/mSecondCaret direct members of this object, instead of pointers? @@ +287,5 @@ > + return NS_OK; > +} > + > +nsresult > +AccessibleCaretManager::TapCaret(const nsPoint& aPoint) Why doesn't this function do anything? @@ +301,5 @@ > + return rv; > +} > + > +nsresult > +AccessibleCaretManager::SelectWordOrShortcut(const nsPoint& aPoint) Please document which functions flush layout and which don't. It's a bit unclear that callers of this function will want to flush layout first. ::: layout/base/AccessibleCaretManager.h @@ +47,5 @@ > + > + virtual nsresult PressCaret(const nsPoint& aPoint); > + virtual nsresult DragCaret(const nsPoint& aPoint); > + virtual nsresult ReleaseCaret(); > + virtual nsresult TapCaret(const nsPoint& aPoint); Document the difference between PressCaret and TpaCaret @@ +48,5 @@ > + virtual nsresult PressCaret(const nsPoint& aPoint); > + virtual nsresult DragCaret(const nsPoint& aPoint); > + virtual nsresult ReleaseCaret(); > + virtual nsresult TapCaret(const nsPoint& aPoint); > + virtual nsresult SelectWordOrShortcut(const nsPoint& aPoint); Document what these points are relative to.
Attachment #8597921 - Flags: review?(roc)
Comment on attachment 8597139 [details] [diff] [review] Part 2.4 - Add AccessibleCaretEventHub (v2) Review of attachment 8597139 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: layout/base/AccessibleCaretEventHub.cpp @@ +394,5 @@ > + !aPresShell->GetCanvasFrame()->GetCustomContentContainer()) { > + return; > + } > + > + nsAutoScriptBlocker scriptBlocker; Why do you need a script blocker here? ::: layout/base/AccessibleCaretEventHub.h @@ +48,5 @@ > +// transitions by giving events, callbacks, and the return values by mocked > +// methods of AccessibleCaretEventHub. See TestAccessibleCaretEventHub.cpp. > +// > +// Besides dealing with real events, AccessibleCaretEventHub also synthesizes > +// fake events such as scroll-end or long-tap providing APZ is not in use. Why do we need to do this? Won't APZ be in use in all the cases we care about? @@ +114,5 @@ > + nsEventStatus HandleKeyboardEvent(WidgetKeyboardEvent* aEvent); > + > + virtual nsPoint GetTouchEventPosition(WidgetTouchEvent* aEvent, > + int32_t aIdentifier) const; > + virtual nsPoint GetMouseEventPosition(WidgetMouseEvent* aEvent) const; Why are these virtual? ::: layout/base/doc/AccessibleCaretEventHubStates.dot @@ +1,3 @@ > +// Steps to generate AccessibleCaretEventHubStates.png > +// 1. Install Graphviz > +// 2. dot -T png -o AccessibleCaretEventHubStates.png AccessibleCaretEventHubStates.dot Very nice :-)
Attachment #8597139 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > Initializing these members at declaration is a new C++ feature. Might be > better for now to keep doing these in the constructor like other classes do. According to [1], we support "member initializers" on all compilers now. Any specific risk not to used this feature? [1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
Comment on attachment 8598469 [details] [diff] [review] Part 6 - Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret (v1) Review of attachment 8598469 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +2237,5 @@ > if (aBuilder->IsBackgroundOnly()) > return; > > + // Skip AccessibleCaret frame if the builder does not build caret. > + if (!aBuilder->IsBuildingCaret()) { Let's not add this here. This is a very hot code. I think we could fix this by modifying nsCanvasFrame instead, to walk the frame children of the custom content container in nsCanvasFrame and call BuildDisplayList on them directly, skipping the caret elements when !IsBuildingCaret.
Attachment #8598469 - Flags: review?(roc) → review-
Summary: Refactor Touch/SelectionCarets → Refactor Touch/SelectionCarets as AccessibleCaret
Attached patch Part 2.2 - Add AccessibleCaret (v5) (obsolete) (deleted) — — Splinter Review
Addressed comment 36. Diff between v4 and v5: * Move one-line functions from cpp to header. * Document the coordinate of Contains() and mImaginaryCaretRect * Document mPresShell, and annotate it with MOZ_NON_OWNING_REF * Document AccessibleCaret does not flush layout or style. Comments not adderssed: 1. About the "none" class when setting appearance. I use the "none" class to add "display: none" (in ua.css) to hide all the elements subtree of AccessibleCaret. If the "none" class is not used, we'll need to set "display: none" to the default style, and add "display: block" to all the Normal/Left/Right css.
Attachment #8597916 - Attachment is obsolete: true
Attachment #8599731 - Flags: review?(roc)
Address comment 37. * Document all public methods * Document AccessibleCaretManager does not flush layout in all public methods prior to performing tasks.
Attachment #8597921 - Attachment is obsolete: true
Attachment #8599735 - Flags: review?(roc)
Re comment 37. Q: Why making mFirstCaret/mSecondCaret unique pointers instead of direct members? A: We do not want to allocate mFirstCaret and mSecondCaret in gtest since mPresShell is a nullptr there. Q: Why doesn't AccessibleCaretManager::TapCaret() do anything? A: Will implement in bug 1155493.
Attached patch Part 2.4 - Add AccessibleCaretEventHub (v3) (obsolete) (deleted) — — Splinter Review
* Document mPresShell * Document why need a script blocker in AccessibleCaretEventHub::Init()
Attachment #8597139 - Attachment is obsolete: true
Attachment #8599738 - Flags: review?(roc)
Re comment 38 Q: Doesn't APZ be in use in all the cases we care about? A: Currently, APZ does not in use on B2G desktop (for Gaia test) and desktop browser. We still need to synthesize scroll-end and long-tap for these platforms. Q: Why are GetTouchEventPosition() and GetMouseEventPosition() virtual? A: To override them in gtest. They will convert the position to be relative to root frame. We need to override them where mPresShell is nullptr in gtest.
Need info about comment 39. Thanks.
Flags: needinfo?(roc)
This part becomes simpler due to new part 2.2 (v5).
Attachment #8598471 - Attachment is obsolete: true
Attachment #8599802 - Flags: review+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #39) > According to [1], we support "member initializers" on all compilers now. Any > specific risk not to used this feature? I've asked on dev-platform whether people think we should use them indiscriminately. Let's see what people say.
Comment on attachment 8599731 [details] [diff] [review] Part 2.2 - Add AccessibleCaret (v5) Review of attachment 8599731 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/AccessibleCaret.h @@ +33,5 @@ > +// All the rect or point are relative to root frame except being specified > +// explicitly. > +// > +// None of the methods in AccessibleCaret will flush layout or style. To ensure > +// that SetPostion() works correctly, the caller must make sure the layout is up SetPosition
Attachment #8599731 - Flags: review?(roc) → review+
Comment on attachment 8599738 [details] [diff] [review] Part 2.4 - Add AccessibleCaretEventHub (v3) Review of attachment 8599738 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/AccessibleCaretEventHub.cpp @@ +395,5 @@ > + return; > + } > + > + // Without this, root frame might become nullptr in AccessibleCaretManager's > + // constructor after mFirstCaret is constructed. Which root frame? Can you explain this in more detail?
Attachment #8599738 - Flags: review?(roc)
Addressed comment 50 by providing more detail about why needs nsAutoScriptBlocker. Full debug log by using rr and reverse-continue to find the cause. Thank you roc! Without using nsAutoScriptBlocker, the frame tree got destroyed after mFirstCaret's InjectCaretElement() was called. https://pastebin.mozilla.org/8832301
Attachment #8599738 - Attachment is obsolete: true
Attachment #8600769 - Flags: review?(roc)
Comment on attachment 8600769 [details] [diff] [review] Part 2.4 - Add AccessibleCaretEventHub (v4) Review of attachment 8600769 [details] [diff] [review]: ----------------------------------------------------------------- It looks to me like the 897852.html testcase reveals a bug that we should fix in a different way --- content mutation event listeners should not fire on changes to the nsCanvasFrame's anonymous content. I'll r+ this so we can get this landed, but please file a new bug about this --- and fix it :-). It's probably just a matter of marking some content native-anonymous I guess...
Attachment #8600769 - Flags: review?(roc) → review+
Blocks: 1161384
Blocks: 1161389
Blocks: 1161392
Re comment 52: Open a follow-up bug for 897852.html testcase in bug 1161384. Move part 6 to bug 1161389 for further invesigation, and move part 7 to bug 1161392 for dependency. roc, thank you for reviewing these huge patches :-) Let's land part 1 ~ 5.
Flags: needinfo?(roc)
Addressed comment 49. Fix a typo.
Attachment #8598469 - Attachment is obsolete: true
Attachment #8599731 - Attachment is obsolete: true
Attachment #8599802 - Attachment is obsolete: true
Attachment #8601282 - Flags: review+
Rebased.
Attachment #8597144 - Attachment is obsolete: true
Attachment #8601283 - Flags: review+
Fix test failure on try, and refine test cases.
Attachment #8592094 - Attachment is obsolete: true
Attachment #8602634 - Flags: review?(mtseng)
Forgot to modify layout/base/tests/marionette/manifest.ini in v2. Now fixed.
Attachment #8602634 - Attachment is obsolete: true
Attachment #8602634 - Flags: review?(mtseng)
Attachment #8602762 - Flags: review?(mtseng)
Attachment #8602762 - Flags: review?(mtseng) → review+
part 3 failed to apply : (eg '1-3,5', or 's' to toggle the sort order between id & patch description) 8 adding 1110039 to series file renamed 1110039 -> Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch applying Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch patching file layout/base/moz.build Hunk #1 FAILED at 144 1 out of 1 hunks FAILED -- saving rejects to file layout/base/moz.build.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh Bug-1110039-Part-3---Add-gtest-for-AccessibleCaret.patch could you take a loo, thanks!
Flags: needinfo?(tlin)
Keywords: checkin-needed
Rebased.
Attachment #8601283 - Attachment is obsolete: true
Attachment #8603814 - Flags: review+
Flags: needinfo?(tlin)
Keywords: checkin-needed
Blocks: 1163490
Blocks: 1163907
This was missing an 'override' annotation on the Name() macro in NS_IMPL_STATE_UTILITIES, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+). I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/71935b9669d9
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: