Refactor Touch/SelectionCarets as AccessibleCaret

RESOLVED FIXED in Firefox 41

Status

()

Core
Selection
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mtseng, Assigned: TYLin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(URL)

Attachments

(9 attachments, 26 obsolete attachments)

3.31 KB, patch
roc
: review+
mtseng
: feedback+
Details | Diff | Splinter Review
2.12 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
778 bytes, patch
TYLin
: review+
Details | Diff | Splinter Review
15.78 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
30.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
148.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.93 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
45.73 KB, patch
mtseng
: review+
Details | Diff | Splinter Review
28.63 KB, 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

Updated

3 years ago
Priority: -- → P3
Depends on: 1114450

Comment 1

3 years ago
This bug will be landed at V3, set as p4 now.
Priority: P3 → P4

Updated

3 years ago
Blocks: 1124074
No longer blocks: 1023688
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 1068525
Blocks: 1130993
Comment hidden (obsolete)
Created attachment 8592089 [details] [diff] [review]
Part 1 - Add nsLayoutUtils::ClampRectToScrollFrames (v1)

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.
Comment hidden (obsolete)
Created attachment 8592092 [details] [diff] [review]
Part 3 - Add gtest for CopyPasteEventHub (v1)
Attachment #8592092 - Flags: feedback?(mtseng)
Created attachment 8592093 [details] [diff] [review]
Part 4 - Hook new classes into the system (v1)

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)
Created attachment 8592094 [details] [diff] [review]
Part 5 - Reuse marionette tests for AccessibleCaret (v1)

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)
Created attachment 8592700 [details] [diff] [review]
Part 2.1 - Add logger facility (v1)
Attachment #8592700 - Flags: feedback?(mtseng)
Created attachment 8592701 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v1)

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)
Created attachment 8592702 [details] [diff] [review]
Part 2.3 - Add CopyPasteManager (v1)

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)
Created attachment 8592703 [details] [diff] [review]
Part 2.4 - Add CopyPasteEventHub (v1)

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)
Created attachment 8592704 [details] [diff] [review]
Part 2.5 - Add all files to build system (v1)
Attachment #8592704 - Flags: feedback?(mtseng)
Comment hidden (obsolete)
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
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.
Created attachment 8597129 [details] [diff] [review]
Part 2.1 - Add logger facility (v2, carry roc r+)

Rename CopyPaste -> AccessibleCaret
Attachment #8592700 - Attachment is obsolete: true
Attachment #8597129 - Flags: review+
Created attachment 8597131 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v2)

Document enum Appearance and other methods suggested in comment 16.
Attachment #8592701 - Attachment is obsolete: true
Attachment #8597131 - Flags: review?(roc)
Created attachment 8597136 [details] [diff] [review]
Part 2.3 - Add AccessibleCaretManager

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.
Created attachment 8597139 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v2)

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)
Created attachment 8597143 [details] [diff] [review]
Part 2.5 - Add all files to build system (v2, carry roc r+)

Rename CopyPaste -> AccessibleCaret
Attachment #8592704 - Attachment is obsolete: true
Attachment #8597143 - Flags: review+
Created attachment 8597144 [details] [diff] [review]
Part 3 - Add gtest for AccessibleCaretEventHub (v2, carry roc r+)

* Rename TestCopyPasteEventHub -> TestAccessibleCaretEventHub
* Style clean-up
Attachment #8592092 - Attachment is obsolete: true
Attachment #8597144 - Flags: review+
Created attachment 8597147 [details] [diff] [review]
Part 4 - Hook new classes into the system (v2, carry roc r+)

Rename CopyPaste -> AccessibleCaret
Attachment #8592093 - Attachment is obsolete: true
Attachment #8597147 - Flags: review+
Created attachment 8597152 [details] [diff] [review]
Part 2.3 - Add AccessibleCaretManager (v2)

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)
Created attachment 8597772 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v3)

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)
Created attachment 8597916 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v4)

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)
Created attachment 8597921 [details] [diff] [review]
Part 2.3 - Add AccessibleCaretManager (v3)

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)
Created attachment 8598469 [details] [diff] [review]
Part 6 - Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret (v1)

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)
Created attachment 8598471 [details] [diff] [review]
Part 7 - Enlarge touch area for AccessibleCaret (v1)

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
Created attachment 8599731 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret  (v5)

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)
Created attachment 8599735 [details] [diff] [review]
Part 2.3 - Add AccessibleCaretManager (v4)

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.
Created attachment 8599738 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v3)

* 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)
Created attachment 8599802 [details] [diff] [review]
Part 7 - Enlarge touch area for AccessibleCaret (v2, carry roc r+)

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)
Created attachment 8600769 [details] [diff] [review]
Part 2.4 - Add AccessibleCaretEventHub (v4)

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+
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)
Created attachment 8601282 [details] [diff] [review]
Part 2.2 - Add AccessibleCaret (v6, carry roc r+)

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+
Created attachment 8601283 [details] [diff] [review]
Part 3 - Add gtest for AccessibleCaretEventHub (v3, carry roc r+)

Rebased.
Attachment #8597144 - Attachment is obsolete: true
Attachment #8601283 - Flags: review+
Created attachment 8602634 [details] [diff] [review]
Part 5 - Reuse marionette tests for AccessibleCaret (v2)

Fix test failure on try, and refine test cases.
Attachment #8592094 - Attachment is obsolete: true
Attachment #8602634 - Flags: review?(mtseng)
Created attachment 8602762 [details] [diff] [review]
Part 5 - Reuse marionette tests for AccessibleCaret (v3)

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+
Marionette tests are all green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5cacbb336a2
Keywords: checkin-needed
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
Created attachment 8603814 [details] [diff] [review]
Part 3 - Add gtest for AccessibleCaretEventHub (v4, carry roc r+)

Rebased.
Attachment #8601283 - Attachment is obsolete: true
Attachment #8603814 - Flags: review+
Flags: needinfo?(tlin)
Keywords: checkin-needed
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.