Closed Bug 1056459 Opened 10 years ago Closed 10 years ago

fire caret move events for anchor jumps as well as focus events

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(1 file, 2 obsolete files)

fire caret move event when document is loaded
12.34 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
without this you can't tell what was scrolled to on platforms that don't have the scrolling start event which is everything other than windows.  It also just makes sense caret moved should work like focus here.

        Attachment #8480101 -
        Flags: review?(surkov.alexander)

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> without this you can't tell what was scrolled to on platforms that don't
> have the scrolling start event which is everything other than windows.

that doesn't much go with the patch, in the patch you just allow early selection change events (caret events of course also). Can you add more details please?

    
    

    
  
Comment on attachment 8480101 [details] [diff] [review]
fire caret move event when document is loaded

Review of attachment 8480101 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I understand test_scroll_caret.xul, it looks like logical copy of test_scroll.xul. Did you miss caret stuff there?

::: accessible/tests/mochitest/events.js
@@ +1756,5 @@
>         "Wrong caret offset for " + prettyName(aEvent.accessible));
>    }
>  }
>  
> +function asyncCaretMoveChecker(aCaretOffset, aTargetOrFunc, aTargetFuncArg)

it doesn't look like you use it somewhere

    
    

    
  
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> > without this you can't tell what was scrolled to on platforms that don't
> > have the scrolling start event which is everything other than windows.
> 
> that doesn't much go with the patch, in the patch you just allow early
> selection change events (caret events of course also). Can you add more
> details please?

we already get notified about the caret move, we just choose to drop it on the floor because the document isn't loaded yet.  What else do you want to know?


(In reply to alexander :surkov from comment #3)
> Comment on attachment 8480101 [details] [diff] [review]
> fire caret move event when document is loaded
> 
> Review of attachment 8480101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure I understand test_scroll_caret.xul, it looks like logical copy
> of test_scroll.xul. Did you miss caret stuff there?

I meant to add use of asyncCaretMove checker, but apparently forgot.

    
    

    
  


  

        Attachment #8480101 -
        Attachment is obsolete: true

        Attachment #8480101 -
        Flags: review?(surkov.alexander)

        Attachment #8480685 -
        Flags: review?(surkov.alexander)

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> we already get notified about the caret move, we just choose to drop it on
> the floor because the document isn't loaded yet.  What else do you want to
> know?

AT problem part, your bug description sounded as you solves some real problem

    
    

    
  
per irc Trev pointed out that's no caret move event when loaded page (like url#anchor) is scrolled to anchor and thus Orca fails to read the page from that point.

    
    

    
  
Comment on attachment 8480685 [details] [diff] [review]
fire caret move event when document is loaded

Review of attachment 8480685 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/events/test_scroll_caret.xul
@@ +58,5 @@
> +        new invokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, tabDocumentAt, 1)
> +      ];
> +
> +      this.unexpectedEventSeq = [
> +        new invokerChecker(EVENT_SCROLLING_START, getAnchorJumpInTabDocument, 1)

you don't test anything new here, should you add caret move events to unexpected sequence?

@@ +97,5 @@
> +    <body xmlns="http://www.w3.org/1999/xhtml">
> +      <a target="_blank"
> +         href="https://bugzilla.mozilla.org/show_bug.cgi?id=691734"
> +         title="Make sure scrolling start event is fired when document receive focus">
> +        Mozilla Bug 691734

pls update it

        Attachment #8480685 -
        Flags: review?(surkov.alexander) → review+

    
    

    
  
(In reply to alexander :surkov from comment #8)
> Comment on attachment 8480685 [details] [diff] [review]
> fire caret move event when document is loaded
> 
> Review of attachment 8480685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/events/test_scroll_caret.xul
> @@ +58,5 @@
> > +        new invokerChecker(EVENT_DOCUMENT_LOAD_COMPLETE, tabDocumentAt, 1)
> > +      ];
> > +
> > +      this.unexpectedEventSeq = [
> > +        new invokerChecker(EVENT_SCROLLING_START, getAnchorJumpInTabDocument, 1)
> 
> you don't test anything new here, should you add caret move events to
> unexpected sequence?


err, but it turns out we actually do get caret move events for back ground tabs.  It sounds Joanie wants these, not sure about other people.

    
    

    
  
This landed as
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc381124409

and it looks like it might've broken "B2G Desktop OS X Opt" Gip tests, with crashes during test_mms_add_subject.py.  Backtrace has "SelectionManager" in it (hence this bug looking suspicious), and looks like this:
{
15:50:08     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
15:50:08     INFO -  Crash address: 0x48
15:50:08     INFO -  Thread 0 (crashed)
15:50:08     INFO -   0  XUL!mozilla::dom::Selection::GetFocusNode() [nsAutoPtr.h:8dc381124409 : 1017 + 0x0]
15:50:08     INFO -      rbx = 0x000000012407d640   r12 = 0x0000000113760cf0
15:50:08     INFO -      r13 = 0x0000000000000000   r14 = 0x000000010afe7e98
15:50:08     INFO -      r15 = 0x000000012407d640   rip = 0x0000000102a80900
15:50:08     INFO -      rsp = 0x00007fff5fbfbc48   rbp = 0x0000000000000000
15:50:08     INFO -      Found by: given as instruction pointer in context
15:50:08     INFO -   1  XUL!mozilla::a11y::SelectionManager::ProcessTextSelChangeEvent(mozilla::a11y::AccEvent*) [SelectionManager.cpp:8dc381124409 : 157 + 0x7]
15:50:08     INFO -      rbx = 0x000000012407d640   r12 = 0x0000000113760cf0
15:50:08     INFO -      r13 = 0x0000000000000000   r14 = 0x000000010afe7e98
15:50:08     INFO -      r15 = 0x000000012407d640   rip = 0x0000000102d4452a
15:50:08     INFO -      rsp = 0x00007fff5fbfbc50   rbp = 0x0000000000000000
15:50:08     INFO -      Found by: call frame info
15:50:08     INFO -   2  XUL!mozilla::a11y::EventQueue::ProcessEventQueue() [EventQueue.cpp:8dc381124409 : 516 + 0xf]
15:50:08     INFO -      rbx = 0x000000012407d640   r12 = 0x000000012407d65c
15:50:08     INFO -      r13 = 0x0000000000000000   r14 = 0x00000001216a1a88
15:50:08     INFO -      r15 = 0x0000000000000002   rip = 0x0000000102d43968
15:50:08     INFO -      rsp = 0x00007fff5fbfbc90   rbp = 0x0000000000000000
15:50:08     INFO -      Found by: call frame info
15:50:08     INFO -   3  XUL!mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) [NotificationController.cpp:8dc381124409 : 271 + 0x7]
15:50:08     INFO -      rbx = 0x0000000000000000   r12 = 0x00000001216a1a80
15:50:08     INFO -      r13 = 0x0000000000000000   r14 = 0x00000001216a1af8
15:50:08     INFO -      r15 = 0x00000001216a1a88   rip = 0x0000000102d4799c
15:50:08     INFO -      rsp = 0x00007fff5fbfbcd0   rbp = 0x0000000000000000
15:50:08     INFO -      Found by: call frame info
15:50:08     INFO -   4  XUL!nsRefreshDriver::Tick(long long, mozilla::TimeStamp) [nsRefreshDriver.cpp:8dc381124409 : 1117 + 0x11]
15:50:08     INFO -      rbx = 0x0000000000000000   r12 = 0x0000000121774800
15:50:08     INFO -      r13 = 0x00000001217749a8   r14 = 0x0000000121774918
15:50:08     INFO -      r15 = 0x0000000000000002   rip = 0x000000010298bcd6
15:50:08     INFO -      rsp = 0x00007fff5fbfbd50   rbp = 0x00000001216a1a80
15:50:08     INFO -      Found by: call frame info
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=47353456&tree=Mozilla-Inbound

    
    

    
  
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1ddba30bf5

For reference, here are two TBPL runs with a bunch of orange Gip runs:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4f68377242c1
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8dc381124409

...and here's the most recent TBPL cycle before tbsaunde's landing which had a green Gip run on OS X:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5c930eeebcac
(I triggered a few more instances of that green Gip run; if they go orange with this same issue, then we'll know this was innocent.)

    
    

    
  


  

        Attachment #8480685 -
        Attachment is obsolete: true

        Attachment #8484410 -
        Flags: review?(surkov.alexander)

    
    

    
  
Comment on attachment 8484410 [details] [diff] [review]
fire caret move event when document is loaded

Review of attachment 8484410 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment answered

::: accessible/base/SelectionManager.cpp
@@ +157,5 @@
> +
> +  // XXX Sometimes we can't get a selection for caretCntr, in that case assume
> +  // event->mSel is correct.
> +  if (!selection)
> +    selection = event->mSel;

there's bunch of craziness in this code: we get selection from node because original selection might be wrong (bug 927159) but if no selection from node then assume that original selection is correct.

1) should we return if node doesn't have selection
2) or if it sounds unsafe in terms of lost events then should we assert

        Attachment #8484410 -
        Flags: review?(surkov.alexander) → review+

    
    

    
  
> ::: accessible/base/SelectionManager.cpp
> @@ +157,5 @@
> > +
> > +  // XXX Sometimes we can't get a selection for caretCntr, in that case assume
> > +  // event->mSel is correct.
> > +  if (!selection)
> > +    selection = event->mSel;
> 
> there's bunch of craziness in this code: we get selection from node because
> original selection might be wrong (bug 927159) but if no selection from node
> then assume that original selection is correct.
> 
> 1) should we return if node doesn't have selection

not sure, since event->mSel *should* be correct it seems pretty reasonable to use it to me, but shrug

> 2) or if it sounds unsafe in terms of lost events then should we assert

I'd add asserts, but it seems strange to me to add asserts that you know will sometimes fire, but I guess we could.

    
    

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

> > 1) should we return if node doesn't have selection
> 
> not sure, since event->mSel *should* be correct it seems pretty reasonable
> to use it to me, but shrug

I agree but it's not as well as acc->DOMSelection() may return null

> > 2) or if it sounds unsafe in terms of lost events then should we assert
> 
> I'd add asserts, but it seems strange to me to add asserts that you know
> will sometimes fire, but I guess we could.

that's for debuggin proposes, that's something that shouldn't happen, right?

    
    

    
  
> > > 2) or if it sounds unsafe in terms of lost events then should we assert
> > 
> > I'd add asserts, but it seems strange to me to add asserts that you know
> > will sometimes fire, but I guess we could.
> 
> that's for debuggin proposes, that's something that shouldn't happen, right?

I still don't like it, but if it makes you happy *shrug*

    
    

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

> > that's for debuggin proposes, that's something that shouldn't happen, right?
> 
> I still don't like it, but if it makes you happy *shrug*

we just add XXX section and some weird code but we don't have any hooks to fix that later.

    
    

    
  
(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> 
> > > that's for debuggin proposes, that's something that shouldn't happen, right?
> > 
> > I still don't like it, but if it makes you happy *shrug*
> 
> we just add XXX section and some weird code but we don't have any hooks to
> fix that later.

Well actually if we add an assert it trips a number of times in the test suite, so lets not do that?

    
    

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

> Well actually if we add an assert it trips a number of times in the test
> suite, so lets not do that?

asserting there sounds like todo for us? then it might be a good idea.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ee4dabe0e241
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: