928504 - reorg text selection change event firing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

fixes for bug 762934, bug 688124 and bug 567571

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

regressions fixed

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 820084 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/AccEvent.cpp b/accessible/src/base/AccEvent.cpp
>--- a/accessible/src/base/AccEvent.cpp
>+++ b/accessible/src/base/AccEvent.cpp
>@@ -113,16 +113,28 @@ AccHideEvent::
> AccShowEvent::
>   AccShowEvent(Accessible* aTarget, nsINode* aTargetNode) :
>   AccMutationEvent(::nsIAccessibleEvent::EVENT_SHOW, aTarget, aTargetNode)
> {
> }
> 
> 
> ////////////////////////////////////////////////////////////////////////////////
>+// AccTextSelChangeEvent
>+////////////////////////////////////////////////////////////////////////////////
>+
>+AccTextSelChangeEvent::AccTextSelChangeEvent(HyperTextAccessible* aTarget,
>+                                             nsISelection* aSelection) :
>+  AccEvent(nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED, aTarget,
>+           eAutoDetect, eCoalesceTextSelChange),
>+  mSel(aSelection) {}
>+
>+AccTextSelChangeEvent::~AccTextSelChangeEvent() { }

So, this needs to be out of line so AccEvent.h doesn't need to include nsISelection.h right?  And AccTextSelChangeEvent only needs to store and hold a ref to a a selection because you can have more than one selection in a document, and its possible one will go away and be replaced by another.

>   EEventRule GetEventRule() const { return mEventRule; }
>   bool IsFromUserInput() const { return mIsFromUserInput; }
>+  EIsFromUserInput FromUserInput() const
>+    { return static_cast<EIsFromUserInput>(mIsFromUserInput); }

if you want to do that you should static assert the enum values are the same as true and false.

>+class AccTextSelChangeEvent : public AccEvent

shouldn't this probably hold the selection bounds at some point somehow?

>+private:
>+  nsCOMPtr<nsISelection> mSel;

since you're forward declaring why not use mozilla::Selection?

>+    {
>+      // Coalesce older event by newer event for the same selection or target.
>+      // Events for same selection may have different targets and vice versa one
>+      // target may be pointed by different selections (for later see
>+      // bug 927159).

why don't you just ifx that first then?

Although actually I'm not totally sure that would save you from storing which selection it was.  Consider what happens if in the same reflow cycle some changes the document selection and the input selection.  However I guess that's really really unlikely and won't go well anyway.

>+      if (event->mEventType == nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED) {
>+        SelectionMgr()->ProcessTextSelChangeEvent(event);

why does that code live in the selection manager more than here?

> void
> SelectionManager::Shutdown()
> {
>   ClearControlSelectionListener();

if it just calls another function why not kill it? or atleast inline it

>+  // Fire selection change event if it's not pure caret-move selection change.
>+  if (sel->GetRangeCount() == 0 || !sel->IsCollapsed())

if the rangeCount != 1 you know its not a caret selection right? it might be nice if the comment related what it tries to do to how code does it some more

>+  nsINode* caretCntrNode =
>+    nsCoreUtils::GetDOMNodeFromDOMPoint(sel->GetFocusNode(),
>+                                        sel->GetFocusOffset());
>+  if (!caretCntrNode)
>+    return;

when can it be null?

>+  HyperTextAccessible* caretCntr = nsAccUtils::GetTextContainer(caretCntrNode);
>+  NS_ASSERTION(caretCntr,
>+               "No text container for focus while there's one for common ancestor?!");

not sure I've ever heard of it firing maybe just remove check and assume its never null?

>+  if (!caretCntr)
>+    return;
>+
>+  nsRefPtr<AccCaretMoveEvent> caretMoveEvent =
>+    new AccCaretMoveEvent(caretCntr, aEvent->FromUserInput());
>+  if (NS_SUCCEEDED(caretCntr->GetCaretOffset(&caretMoveEvent->mCaretOffset)) &&
>+      caretMoveEvent->mCaretOffset != -1) {

seems like it would be nicer to pass offset to constructor and check offset before creating object.

>+  HyperTextAccessible* text = nsAccUtils::GetTextContainer(cntrNode);

maybe this should move to be method of DocAccessible?

>+  } else if (selection->GetType() == nsISelectionController::SELECTION_SPELLCHECK) {
>+    // XXX: fire an event for accessible of container node of the focus/anchor
>+    // selection range. If spellchecking is enabled then we will fire the number

huh? afaik spell check range doesn't have focused range, its just hack to contain set of ranges.

>+    // of events for the same accessible for newly appended range of
>+    // the selection (for every misspelled word). If spellchecking is disabled

what is this number of events stuff supposed to mean? I don't get it

>+SelectionManager::GetCaretRectIn(HyperTextAccessible* aTextAccessible,
>+                                 nsIWidget** aWidget)

maybe it should be method of HyperTextAccessible?

>+  nsIntRect caretRect =
>+    SelectionMgr()->GetCaretRectIn(aAccessible->AsHyperText(), &widget);
>+  HWND caretWnd;
>   if (caretRect.IsEmpty() || !(caretWnd = (HWND)widget->GetNativeData(NS_NATIVE_WINDOW))) {
>     return;

maybe it would be easier to handle event target being not hypertext here than pass null to SelectionManager? (can that ever happen anyway?)

>+    /**
>+     * Do tests.
>+     */

seems like useless comment

>+      gQueue.push(new synthDownKey("c1", new textSelectionChecker("c1", 0, 1), { shiftKey: true }));
>+      gQueue.push(new synthDownKey("c1", new textSelectionChecker("c1", 0, 2), { shiftKey: true }));

bounds look funny because of embedded objects right?

>+++ b/accessible/tests/mochitest/hittest/test_browser.html
>+      getNode("hittest").scrollIntoView(true);

is that really related?

>+++ b/accessible/tests/mochitest/hittest/test_menu.xul
> 
>+      getNode("mi_file1").scrollIntoView(true);

related?

>       this.eventSeq = [
>-        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, aID)
>+        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, document)

why?

Comment 3

[Trevor Saunders (:tbsaunde)]

another way you could hanlde the different selections would be to add a generation to the event, and keep a global holding the current generation which would get incremented each time the selection controller being listened to changes.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> So, this needs to be out of line 

what do you mean?

> so AccEvent.h doesn't need to include
> nsISelection.h right? 

yes

>  And AccTextSelChangeEvent only needs to store and
> hold a ref to a a selection because you can have more than one selection in
> a document,

right

>  and its possible one will go away and be replaced by another.

it shouldn't be

> >   EEventRule GetEventRule() const { return mEventRule; }
> >   bool IsFromUserInput() const { return mIsFromUserInput; }
> >+  EIsFromUserInput FromUserInput() const
> >+    { return static_cast<EIsFromUserInput>(mIsFromUserInput); }
> 
> if you want to do that you should static assert the enum values are the same
> as true and false.

ok

> >+class AccTextSelChangeEvent : public AccEvent
> 
> shouldn't this probably hold the selection bounds at some point somehow?

I miss a possible reason

> >+private:
> >+  nsCOMPtr<nsISelection> mSel;
> 
> since you're forward declaring why not use mozilla::Selection?

I get nsISelection from nsISelectionListener and just store it, I could but I don't see a lot of sense

> >+    {
> >+      // Coalesce older event by newer event for the same selection or target.
> >+      // Events for same selection may have different targets and vice versa one
> >+      // target may be pointed by different selections (for later see
> >+      // bug 927159).
> 
> why don't you just ifx that first then?

Unfortunately I don't always have time to make things right. If you can help with bug then it'd be awesome.

> Although actually I'm not totally sure that would save you from storing
> which selection it was.  Consider what happens if in the same reflow cycle
> some changes the document selection and the input selection.  However I
> guess that's really really unlikely and won't go well anyway.

yep, we'll likely have a problem

> >+      if (event->mEventType == nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED) {
> >+        SelectionMgr()->ProcessTextSelChangeEvent(event);
> 
> why does that code live in the selection manager more than here?

I'm not sure I like organization, I followed the pattern created by FocusMgr().

> > void
> > SelectionManager::Shutdown()
> > {
> >   ClearControlSelectionListener();
> 
> if it just calls another function why not kill it? or atleast inline it

inlined

> >+  // Fire selection change event if it's not pure caret-move selection change.
> >+  if (sel->GetRangeCount() == 0 || !sel->IsCollapsed())
> 
> if the rangeCount != 1 you know its not a caret selection right? it might be
> nice if the comment related what it tries to do to how code does it some more

caret is one collapsed range only, what kind of comment would help here?

> >+  nsINode* caretCntrNode =
> >+    nsCoreUtils::GetDOMNodeFromDOMPoint(sel->GetFocusNode(),
> >+                                        sel->GetFocusOffset());
> >+  if (!caretCntrNode)
> >+    return;
> 
> when can it be null?

when no caret

> 
> >+  HyperTextAccessible* caretCntr = nsAccUtils::GetTextContainer(caretCntrNode);
> >+  NS_ASSERTION(caretCntr,
> >+               "No text container for focus while there's one for common ancestor?!");
> 
> not sure I've ever heard of it firing maybe just remove check and assume its
> never null?

I'd better be on safe side here, it worries me due to some reason. If you want then I can remove it of course.

> >+  if (!caretCntr)
> >+    return;
> >+
> >+  nsRefPtr<AccCaretMoveEvent> caretMoveEvent =
> >+    new AccCaretMoveEvent(caretCntr, aEvent->FromUserInput());
> >+  if (NS_SUCCEEDED(caretCntr->GetCaretOffset(&caretMoveEvent->mCaretOffset)) &&
> >+      caretMoveEvent->mCaretOffset != -1) {
> 
> seems like it would be nicer to pass offset to constructor and check offset
> before creating object.

ok, I can fix that I think.

> >+  HyperTextAccessible* text = nsAccUtils::GetTextContainer(cntrNode);
> 
> maybe this should move to be method of DocAccessible?

probably but we don't really have a document here, so we have few nsAccUtils methods for cases like this

> >+  } else if (selection->GetType() == nsISelectionController::SELECTION_SPELLCHECK) {
> >+    // XXX: fire an event for accessible of container node of the focus/anchor
> >+    // selection range. If spellchecking is enabled then we will fire the number
> 
> huh? afaik spell check range doesn't have focused range, its just hack to
> contain set of ranges.

apparently it has, new code is sort of equivalent to old code at least what relates to focus/anchor range (mochitests passes)

> >+    // of events for the same accessible for newly appended range of
> >+    // the selection (for every misspelled word). If spellchecking is disabled
> 
> what is this number of events stuff supposed to mean? I don't get it

I don't know what it means since events for the same target should be coalesced, I think I can remove this part of comment.

> >+SelectionManager::GetCaretRectIn(HyperTextAccessible* aTextAccessible,
> >+                                 nsIWidget** aWidget)
> 
> maybe it should be method of HyperTextAccessible?

seems like a good idea, should I move it?

> >+  nsIntRect caretRect =
> >+    SelectionMgr()->GetCaretRectIn(aAccessible->AsHyperText(), &widget);
> >+  HWND caretWnd;
> >   if (caretRect.IsEmpty() || !(caretWnd = (HWND)widget->GetNativeData(NS_NATIVE_WINDOW))) {
> >     return;
> 
> maybe it would be easier to handle event target being not hypertext here
> than pass null to SelectionManager? (can that ever happen anyway?)

in case of focus event - yes, so get a hyperText local varialbe and add check?

> >+      gQueue.push(new synthDownKey("c1", new textSelectionChecker("c1", 0, 1), { shiftKey: true }));
> >+      gQueue.push(new synthDownKey("c1", new textSelectionChecker("c1", 0, 2), { shiftKey: true }));
> 
> bounds look funny because of embedded objects right?

right

> >+++ b/accessible/tests/mochitest/hittest/test_browser.html
> >+      getNode("hittest").scrollIntoView(true);
> 
> is that really related?

somehow magically, because of synthKey which scrolls I get failures in hittesting, so scrolling those tests fix the problem

> 
> >+++ b/accessible/tests/mochitest/hittest/test_menu.xul
> > 
> >+      getNode("mi_file1").scrollIntoView(true);
> 
> related?

yes

> >       this.eventSeq = [
> >-        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, aID)
> >+        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, document)
> 
> why?

when no selection (empty ranges) then we don't have any idea where it was, until we have a cache a document is best our guess

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > So, this needs to be out of line 
> 
> what do you mean?

not in header

> 
> > so AccEvent.h doesn't need to include
> > nsISelection.h right? 
> 
> yes
> 
> >  And AccTextSelChangeEvent only needs to store and
> > hold a ref to a a selection because you can have more than one selection in
> > a document,
> 
> right
> 
> >  and its possible one will go away and be replaced by another.
> 
> it shouldn't be

I mean in memory and I don't see how you can be sure of that.

> > >+class AccTextSelChangeEvent : public AccEvent
> > 
> > shouldn't this probably hold the selection bounds at some point somehow?
> 
> I miss a possible reason

I sort of thought atk event wants to know how things change.

> 
> > >+private:
> > >+  nsCOMPtr<nsISelection> mSel;
> > 
> > since you're forward declaring why not use mozilla::Selection?
> 
> I get nsISelection from nsISelectionListener and just store it, I could but
> I don't see a lot of sense

just static cast in one place instead of couple.

> > Although actually I'm not totally sure that would save you from storing
> > which selection it was.  Consider what happens if in the same reflow cycle
> > some changes the document selection and the input selection.  However I
> > guess that's really really unlikely and won't go well anyway.
> 
> yep, we'll likely have a problem

I guess we can not worry about it till other bug is fixed.  What do you think of my proposal to just keep selection generation instead of actual selection?

> 
> > >+      if (event->mEventType == nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED) {
> > >+        SelectionMgr()->ProcessTextSelChangeEvent(event);
> > 
> > why does that code live in the selection manager more than here?
> 
> I'm not sure I like organization, I followed the pattern created by
> FocusMgr().

I'd say if it makes more sense do something different.

> > >+  // Fire selection change event if it's not pure caret-move selection change.
> > >+  if (sel->GetRangeCount() == 0 || !sel->IsCollapsed())
> > 
> > if the rangeCount != 1 you know its not a caret selection right? it might be
> > nice if the comment related what it tries to do to how code does it some more
> 
> caret is one collapsed range only, what kind of comment would help here?

maybe rewrite check to

rangeCount != 1 || !range.IsCollapsed()

> probably but we don't really have a document here, so we have few nsAccUtils
> methods for cases like this

yeah, just nice to clean up some day.

> > >+SelectionManager::GetCaretRectIn(HyperTextAccessible* aTextAccessible,
> > >+                                 nsIWidget** aWidget)
> > 
> > maybe it should be method of HyperTextAccessible?
> 
> seems like a good idea, should I move it?

I think it would be nice.

> 
> > >+  nsIntRect caretRect =
> > >+    SelectionMgr()->GetCaretRectIn(aAccessible->AsHyperText(), &widget);
> > >+  HWND caretWnd;
> > >   if (caretRect.IsEmpty() || !(caretWnd = (HWND)widget->GetNativeData(NS_NATIVE_WINDOW))) {
> > >     return;
> > 
> > maybe it would be easier to handle event target being not hypertext here
> > than pass null to SelectionManager? (can that ever happen anyway?)
> 
> in case of focus event - yes, so get a hyperText local varialbe and add
> check?

yeah

> > >+++ b/accessible/tests/mochitest/hittest/test_browser.html
> > >+      getNode("hittest").scrollIntoView(true);
> > 
> > is that really related?
> 
> somehow magically, because of synthKey which scrolls I get failures in
> hittesting, so scrolling those tests fix the problem

but why did scrolling change by this patch?

> > 
> > >+++ b/accessible/tests/mochitest/hittest/test_menu.xul
> > > 
> > >+      getNode("mi_file1").scrollIntoView(true);
> > 
> > related?
> 
> yes

I assume same as above.

> > >       this.eventSeq = [
> > >-        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, aID)
> > >+        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, document)
> > 
> > why?
> 
> when no selection (empty ranges) then we don't have any idea where it was,
> until we have a cache a document is best our guess

but isn't the point we're removing a existing selection?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > So, this needs to be out of line 
> > 
> > what do you mean?
> 
> not in header

why?

> > >  and its possible one will go away and be replaced by another.
> > 
> > it shouldn't be
> 
> I mean in memory and I don't see how you can be sure of that.

I don't know what is life cycle of selection object, I assumed it's the same as the object it's created for

> > > shouldn't this probably hold the selection bounds at some point somehow?
> > 
> > I miss a possible reason
> 
> I sort of thought atk event wants to know how things change.

ah, then yes. Take some time please to file a bug? (it seems I'm not familiar with this part of ATK spec)

> > > >+private:
> > > >+  nsCOMPtr<nsISelection> mSel;
> > > 
> > > since you're forward declaring why not use mozilla::Selection?
> > 
> > I get nsISelection from nsISelectionListener and just store it, I could but
> > I don't see a lot of sense
> 
> just static cast in one place instead of couple.

yes but why to bother?

> > > Although actually I'm not totally sure that would save you from storing
> > > which selection it was.  Consider what happens if in the same reflow cycle
> > > some changes the document selection and the input selection.  However I
> > > guess that's really really unlikely and won't go well anyway.
> > 
> > yep, we'll likely have a problem
> 
> I guess we can not worry about it till other bug is fixed.  What do you
> think of my proposal to just keep selection generation instead of actual
> selection?

sorry, what selection generation is?

> > > why does that code live in the selection manager more than here?
> > 
> > I'm not sure I like organization, I followed the pattern created by
> > FocusMgr().
> 
> I'd say if it makes more sense do something different.

I appreciate if you put an idea into a bug

> > > >+  // Fire selection change event if it's not pure caret-move selection change.
> > > >+  if (sel->GetRangeCount() == 0 || !sel->IsCollapsed())
> > > 
> > > if the rangeCount != 1 you know its not a caret selection right? it might be
> > > nice if the comment related what it tries to do to how code does it some more
> > 
> > caret is one collapsed range only, what kind of comment would help here?
> 
> maybe rewrite check to
> 
> rangeCount != 1 || !range.IsCollapsed()

ok

> > > >+SelectionManager::GetCaretRectIn(HyperTextAccessible* aTextAccessible,
> > > >+                                 nsIWidget** aWidget)
> > > 
> > > maybe it should be method of HyperTextAccessible?
> > 
> > seems like a good idea, should I move it?
> 
> I think it would be nice.

ok


> > > maybe it would be easier to handle event target being not hypertext here
> > > than pass null to SelectionManager? (can that ever happen anyway?)
> > 
> > in case of focus event - yes, so get a hyperText local varialbe and add
> > check?
> 
> yeah

ok

> > > >+++ b/accessible/tests/mochitest/hittest/test_browser.html
> > > >+      getNode("hittest").scrollIntoView(true);
> > > 
> > > is that really related?
> > 
> > somehow magically, because of synthKey which scrolls I get failures in
> > hittesting, so scrolling those tests fix the problem
> 
> but why did scrolling change by this patch?

I added some tests doing scrolling before hittesting and got failures, I assumed there's something wrong with hit testing area visibility so made sure to scroll it into view and that helped. I didn't do any debugging but since I didn't touch any hit testing code then I'm more or less confident that was a cause.

> > > >+      getNode("mi_file1").scrollIntoView(true);
> > > 
> > > related?
> > 
> > yes
> 
> I assume same as above.

right

> > > >       this.eventSeq = [
> > > >-        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, aID)
> > > >+        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, document)
> > > 
> > > why?
> > 
> > when no selection (empty ranges) then we don't have any idea where it was,
> > until we have a cache a document is best our guess
> 
> but isn't the point we're removing a existing selection?

yes, that's the same, right?

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #4)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > 
> > > > So, this needs to be out of line 
> > > 
> > > what do you mean?
> > 
> > not in header
> 
> why?

I don't understand question

> > > >  and its possible one will go away and be replaced by another.
> > > 
> > > it shouldn't be
> > 
> > I mean in memory and I don't see how you can be sure of that.
> 
> I don't know what is life cycle of selection object, I assumed it's the same
> as the object it's created for

I don't think its safe to make assumptions about it here

> > > > shouldn't this probably hold the selection bounds at some point somehow?
> > > 
> > > I miss a possible reason
> > 
> > I sort of thought atk event wants to know how things change.
> 
> ah, then yes. Take some time please to file a bug? (it seems I'm not
> familiar with this part of ATK spec)

ok, I'll try and look.

> > > > >+private:
> > > > >+  nsCOMPtr<nsISelection> mSel;
> > > > 
> > > > since you're forward declaring why not use mozilla::Selection?
> > > 
> > > I get nsISelection from nsISelectionListener and just store it, I could but
> > > I don't see a lot of sense
> > 
> > just static cast in one place instead of couple.
> 
> yes but why to bother?

just looks a little nicer, but I don't really care

> > > > Although actually I'm not totally sure that would save you from storing
> > > > which selection it was.  Consider what happens if in the same reflow cycle
> > > > some changes the document selection and the input selection.  However I
> > > > guess that's really really unlikely and won't go well anyway.
> > > 
> > > yep, we'll likely have a problem
> > 
> > I guess we can not worry about it till other bug is fixed.  What do you
> > think of my proposal to just keep selection generation instead of actual
> > selection?
> 
> sorry, what selection generation is?

comment 3?

> > > > why does that code live in the selection manager more than here?
> > > 
> > > I'm not sure I like organization, I followed the pattern created by
> > > FocusMgr().
> > 
> > I'd say if it makes more sense do something different.
> 
> I appreciate if you put an idea into a bug

I'd probably just inline it into the caller

> > > > >+++ b/accessible/tests/mochitest/hittest/test_browser.html
> > > > >+      getNode("hittest").scrollIntoView(true);
> > > > 
> > > > is that really related?
> > > 
> > > somehow magically, because of synthKey which scrolls I get failures in
> > > hittesting, so scrolling those tests fix the problem
> > 
> > but why did scrolling change by this patch?
> 
> I added some tests doing scrolling before hittesting and got failures, I
> assumed there's something wrong with hit testing area visibility so made
> sure to scroll it into view and that helped. I didn't do any debugging but
> since I didn't touch any hit testing code then I'm more or less confident
> that was a cause.

sounds kind of fragile... maybe it would be better to unscroll in the tests that scroll?

> > > > >       this.eventSeq = [
> > > > >-        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, aID)
> > > > >+        new invokerChecker(EVENT_TEXT_SELECTION_CHANGED, document)
> > > > 
> > > > why?
> > > 
> > > when no selection (empty ranges) then we don't have any idea where it was,
> > > until we have a cache a document is best our guess
> > 
> > but isn't the point we're removing a existing selection?
> 
> yes, that's the same, right?

then should we be able to find a better target of selection change event?

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 822088 [details] [diff] [review]
patch

>+++ b/accessible/src/base/Asserts.cpp
>@@ -1,14 +1,15 @@
>+static_assert(static_cast<bool>(eNoUserInput) == false &&
>+              static_cast<bool>(eFromUserInput) == true,
>+              "EIsFromUserInput cannot be casted to bool");

wouldn't it be nicer in AccEvent. h / cpp?

>+HyperTextAccessible::GetCaretRect(nsIWidget** aWidget)
>+{
>+  NS_ENSURE_TRUE(aWidget, nsIntRect());
>+  *aWidget = nullptr;

why bother with null check?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > > > So, this needs to be out of line 
> > > > 
> > > > what do you mean?
> > > 
> > > not in header
> > 
> > why?
> 
> I don't understand question

why it needs to be out of line

> > > > >  and its possible one will go away and be replaced by another.
> > > > 
> > > > it shouldn't be
> > > 
> > > I mean in memory and I don't see how you can be sure of that.
> > 
> > I don't know what is life cycle of selection object, I assumed it's the same
> > as the object it's created for
> 
> I don't think its safe to make assumptions about it here

what do you suggest?

> > > I guess we can not worry about it till other bug is fixed.  What do you
> > > think of my proposal to just keep selection generation instead of actual
> > > selection?
> > 
> > sorry, what selection generation is?
> 
> comment 3?

I missed the idea, sorry

> > > > > why does that code live in the selection manager more than here?
> > > > 
> > > > I'm not sure I like organization, I followed the pattern created by
> > > > FocusMgr().
> > > 
> > > I'd say if it makes more sense do something different.
> > 
> > I appreciate if you put an idea into a bug
> 
> I'd probably just inline it into the caller

I'm not sure about this one

> > I added some tests doing scrolling before hittesting and got failures, I
> > assumed there's something wrong with hit testing area visibility so made
> > sure to scroll it into view and that helped. I didn't do any debugging but
> > since I didn't touch any hit testing code then I'm more or less confident
> > that was a cause.
> 
> sounds kind of fragile... maybe it would be better to unscroll in the tests
> that scroll?

that'd be probably harder

> > > > when no selection (empty ranges) then we don't have any idea where it was,
> > > > until we have a cache a document is best our guess
> > > 
> > > but isn't the point we're removing a existing selection?
> > 
> > yes, that's the same, right?
> 
> then should we be able to find a better target of selection change event?

I'm not sure how to proceed here

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 822088 [details] [diff] [review]
> patch
> 
> >+++ b/accessible/src/base/Asserts.cpp
> >@@ -1,14 +1,15 @@
> >+static_assert(static_cast<bool>(eNoUserInput) == false &&
> >+              static_cast<bool>(eFromUserInput) == true,
> >+              "EIsFromUserInput cannot be casted to bool");
> 
> wouldn't it be nicer in AccEvent. h / cpp?

I can move it if you prefer

> >+HyperTextAccessible::GetCaretRect(nsIWidget** aWidget)
> >+{
> >+  NS_ENSURE_TRUE(aWidget, nsIntRect());
> >+  *aWidget = nullptr;
> 
> why bother with null check?

ok

Comment 12

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/32564a62dee7

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/32564a62dee7