Closed Bug 256244 Opened 20 years ago Closed 14 years ago

Unable to navigate DOM Inspector without mouse (mouselessly)

Categories

(Other Applications :: DOM Inspector, defect)

Product:
Other Applications
Component:
DOM Inspector
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: daniel.oconnor, Assigned: crussell)

References

Details

(Keywords: access, Whiteboard: DUPEME or send me back to firefox)

Attachments

(3 files, 7 obsolete files)

put viewer list and viewer menu toolbarbuttons in the tab order for sidebar.xul and object.xul
3.35 KB, patch
neil
: review+
Details | Diff | Splinter Review
phase 1, address comment 24 and 25
15.09 KB, patch
neil
: review+
Details | Diff | Splinter Review
phase 2, uncheck previously checked items, handling unexpected occurrences better
7.19 KB, patch
neil
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.8

DOM Inspector.
You can navigate this well, but the right click menu to insert attribute+value
is inaccessable. Workaround exists under edit menu.

Additionally, you cannot change to "javascript"/alternate modes ... suggest menu
items under view menu for these. Unable to further check other areas of this
dialog due to lack of mouse.


Reproducible: Always
Steps to Reproduce:
1. Open DOM Inspector
2. Navigate without mouse (tab)
3.

Actual Results:  
Unable to select UI components

Expected Results:  
All components should be available to gain focus on.
"You can navigate this well, but the right click menu to insert attribute+value
is inaccessable."

Confirming
->DOM Inspector
Status: UNCONFIRMED → NEW
Component: Accessibility → DOM Inspector
Ever confirmed: true
Keywords: access
Product: Firefox → Browser
Version: unspecified → Trunk
Whoops, I meant I agree with
>Additionally, you cannot change to "javascript"/alternate modes ... suggest menu
>items under view menu for these.

The context menu containing insert attribute *is* accessible a least under Mozilla.
Assignee: aaronleventhal → dom-inspector
QA Contact: bugzilla → timeless
Whiteboard: DUPEME or send me back to firefox
Product: Core → Other Applications
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
Attached patch cleaunp (obsolete) — — Splinter Review 
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED

        Attachment #497174 -
        Flags: review?(neil)

    
    

    
  


  
Cache the last entries delivered during rebuildViewerList to prevent excessive calls to findViewersForObject; before rebuilding the Document Viewer and Object Viewer popups in response to subjectChange, check to see if we actually need to rebuild the popups.

        Attachment #497178 -
        Flags: review?(neil)

    
    

    
  


  
It's safe to stop sourcing inspectorWindow.css in object.xul.  It's supposed to be for styling the full (inspector.xul) "inspector app" window, and it none of the extra rules in there apply to anything in object.xul (because the elements it affects are only in the full inspector).

This explicitly prevents the viewer list and viewer menu toolbarbuttons from being in the tab order in the full inspector, because there exists equivalent functionality from the new popups accessible from the View menu.

        Attachment #497180 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 497174 [details] [diff] [review]
cleaunp

I thought the whole point of this was that 1.9.x doesn't have that interface?

    
    

    
  
Comment on attachment 497180 [details] [diff] [review]
put viewer list and viewer menu toolbarbuttons in the tab order for sidebar.xul and object.xul

It might make more sense to combine the focus rules in both cases.

        Attachment #497180 -
        Flags: review?(neil) → review+

    
    

    
  
(In reply to comment #6)
> Cache the last entries delivered during rebuildViewerList to prevent excessive
> calls to findViewersForObject; before rebuilding the Document Viewer and Object
> Viewer popups in response to subjectChange, check to see if we actually need to
> rebuild the popups.
I wouldn't say they're excessive ;-)

[I did notice that, when inspecting DOM Inspector itself, every time you switch document viewer, it appends " - DOM Inspector" to the title...]

    
    

    
  
Weird. The first time I tried this, my Object Viewer menu was disabled and I was unable to enable it. But I can't reproduce it, or remember the steps.

    
    

    
  
Comment on attachment 497176 [details] [diff] [review]
Add "Document Viewer" and "Object Viewer" menus to the View menu

>+      <method name="updateViewerListSelection">
>+        <parameter name="aPopup"/>
>+        <body><![CDATA[
>+          var entry = this.mCurrentEntry;
>+          for (let i = 0, n = aPopup.childNodes.length; i < n; ++i) {
>+            let kid = aPopup.childNodes[i];
>+            if (kid.getAttribute("viewerListEntry") == entry) {
Can you not use
aPopup.getElementsByAttribute("viewerListEntry", this.mCurrentEntry) ?

    
    

    
  
Comment on attachment 497178 [details] [diff] [review]
Employ some caching regarding the entries in the popups

If you aren't going to rebuild the viewer list because the subject hasn't changed, why bother caching the entries?

    
    

    
  
Comment on attachment 497176 [details] [diff] [review]
Add "Document Viewer" and "Object Viewer" menus to the View menu

>+          var entries =
>+            aEntries === undefined ?
>+              this.registry.findViewersForObject(subject, this.id,
>+                                                 linkedViewer) :
>+              aEntries;
Why not
var entries = aEntries || this.registry.findViewersForObject(...); ?

Or instead of passing them in from the caller, return them to the caller?

    
    

    
  
(In reply to comment #8)
> Comment on attachment 497174 [details] [diff] [review]
> cleaunp
> 
> I thought the whole point of this was that 1.9.x doesn't have that interface?

It doesn't, but I don't get any errors about it.

(In reply to comment #10)
> (In reply to comment #6)
> > Cache the last entries delivered during rebuildViewerList to prevent excessive
> > calls to findViewersForObject; before rebuilding the Document Viewer and Object
> > Viewer popups in response to subjectChange, check to see if we actually need to
> > rebuild the popups.
> I wouldn't say they're excessive ;-)

Well, consider what happens if you inspect a new document or switch to a new document viewer (without attachment 497178 [details] [diff] [review]).  There'd be four calls to findViewersForObject—one for each menupopup.

> [I did notice that, when inspecting DOM Inspector itself, every time you switch
> document viewer, it appends " - DOM Inspector" to the title...]

Bug 400095.

(In reply to comment #13)
> Comment on attachment 497178 [details] [diff] [review]
> Employ some caching regarding the entries in the popups
> 
> If you aren't going to rebuild the viewer list because the subject hasn't
> changed, why bother caching the entries?

There are two scenarios coming into play here.  One is described in the comment about subjectChange not implying a real subject change for the panel.  The other is when the panel subject actually does change.  Say you select a new node in the DOM Nodes viewer; the object panel binding will call rebuildViewerList once (which will result in a findViewersForObject call), then inspector.js will call rebuildViewerList once after it receives the subjectChange (which will just use the cached entries from the binding's own earlier call).

I'm not actually sure of the merits of checking that the subject actually did change.  Going ahead with a call to rebuildViewerList would eventually just hit the object–entry cache.  I figure that fewer DOM manipulations and a shorter call stack when possible might be worth it.  On the other hand, the mLastKnown* objects-that-want-to-be-pointers aren't exactly pretty.  Thoughts?

(In reply to comment #14)
> Comment on attachment 497176 [details] [diff] [review]
> Add "Document Viewer" and "Object Viewer" menus to the View menu
> 
> >+          var entries =
> >+            aEntries === undefined ?
> >+              this.registry.findViewersForObject(subject, this.id,
> >+                                                 linkedViewer) :
> >+              aEntries;
> Why not
> var entries = aEntries || this.registry.findViewersForObject(...); ?

I guess because after giving it not much thought, I felt a null aEntries should take the other path.

> Or instead of passing them in from the caller, return them to the caller?

Better.

    
    

    
  
(In reply to comment #15)
> the mLastKnown* objects-that-want-to-be-pointers aren't exactly pretty.

Although, if we stuck with that, it could be repurposed for solving the window title that grows with every viewer change, instead of my terrible suggestion from bug 400095, comment 14.

    
    

    
  
Here's another possibility to add to the mix:
Each panel already tracks its subject. When its subject changes it rebuilds its popup. We could early-return if the subject didn't really change. We could then cache the viewer entries in the panel. The main menu version would then rebuild itself using the cached entries. (It would still be extra rebuilding though.)

    
    

    
  
(In reply to comment #17)
> Here's another possibility to add to the mix:
> Each panel already tracks its subject. When its subject changes it rebuilds its
> popup. We could early-return if the subject didn't really change.

If you're still referring to the panel code in the last statement, that's not necessary.  We have to discern between real subject changes and new-viewer-fired-subjectChange-for-same-old-subject in inspector.js, because it rebuilds based on subjectChange being dispatched.  The panel binding does not.  It rebuilds in its |subject| setter, so in almost* all cases, the panel binding code only ever rebuilds the viewer list when the subject is actually changing.

*In theory, I guess if |subject| is explicitly set to something which is already the subject, it'll try to rebuild the viewer list, even though it doesn't need to.  A DOM Nodes tree rebuild based on toggling anonymous nodes or preproccessing instructions or some other pref can cause this, I think.  In any case, it'll hit the object–entry cache.

In my next patch, I think I will change setSubject to sniff out this case, which should result in faster DOM Nodes tree rebuilds after toggling a pref.

> We could then
> cache the viewer entries in the panel. The main menu version would then rebuild
> itself

In what way?

If I understand correctly, I think this is the original approach I wanted to take.  I didn't go for that route, though, because I wanted a shared rebuilding function, rather than duplicating the rebuild loops in inspector.js.

    
    

    
  
(In reply to comment #18)
> (In reply to comment #17)
> > Here's another possibility to add to the mix:
> > Each panel already tracks its subject. When its subject changes it rebuilds its
> > popup. We could early-return if the subject didn't really change.
> If you're still referring to the panel code in the last statement, that's not
> necessary.  We have to discern between real subject changes and
> new-viewer-fired-subjectChange-for-same-old-subject in inspector.js, because it
> rebuilds based on subjectChange being dispatched.  The panel binding does not. 
> It rebuilds in its |subject| setter, so in almost* all cases, the panel binding
> code only ever rebuilds the viewer list when the subject is actually changing.
Ah, so I think I must have confused myself because I was actually seeing the right panel's subject changing. Sorry about that.

> *In theory, I guess if |subject| is explicitly set to something which is
> already the subject, it'll try to rebuild the viewer list, even though it
> doesn't need to.  A DOM Nodes tree rebuild based on toggling anonymous nodes or
> preproccessing instructions or some other pref can cause this, I think.
Doesn't seem to.

> > We could then cache the viewer entries in the panel.
> > The main menu version would then rebuild itself
> In what way?
panel.rebuildViewerList(popup) as before, but with the entries having been cached by setSubject so you don't need to find them again.

    
    

    
  
(In reply to comment #19)
> > *In theory, I guess if |subject| is explicitly set to something which is
> > already the subject, it'll try to rebuild the viewer list, even though it
> > doesn't need to.  A DOM Nodes tree rebuild based on toggling anonymous nodes or
> > preproccessing instructions or some other pref can cause this, I think.
> Doesn't seem to.

It does.  Select a node that's not in an anonymous subtree, for example, and toggle anonymous nodes off.  At the end of rebuilding the tree when endSelectionBatch is called, the object in mPendingSelection will be the same as the object panel's subject.  changeSelection will dispatch a selectionChange, and updateLinkedSubject will set the object panel's subject to it.

> > > We could then cache the viewer entries in the panel.
> > > The main menu version would then rebuild itself
> > In what way?
> panel.rebuildViewerList(popup) as before, but with the entries having been
> cached by setSubject so you don't need to find them again.

So how is that different?  It's just doing the caching in setSubject instead of in rebuildViewerList?

    
    

    
  


  
mLastKnownObjectPanelSubject and mLastKnownDocPanelSubject are still around

        Attachment #497178 -
        Attachment is obsolete: true

        Attachment #498359 -
        Flags: review?(neil)

        Attachment #497178 -
        Flags: review?(neil)

    
    

    
  
(In reply to comment #20)
> At the end of rebuilding the tree when endSelectionBatch is called, the
> object in mPendingSelection will be the same as the object panel's subject.
I'm getting my panels mixed up yet again. Sorry about that.

> So how is that different?  It's just doing the caching in setSubject instead of
> in rebuildViewerList?
Well, it's different in that the code for doing the caching in setSubject looks much nicer than the code for doing the caching in rebuildViewerList ;-)

    
    

    
  
Comment on attachment 498359 [details] [diff] [review]
Cache entries in setSubject instead of in rebuildViewerList

>           if (aEvent.target == this.mObjectPanel.viewer) {
>             panel = this.mObjectPanel;
>             mpp = this.mObjectViewerListPopup;
>+            last = this.mLastKnownObjectPanelSubject;
>           }
...
>         if (panel) {
>-          panel.rebuildViewerList(mpp);
>+          if (last.value != aEvent.subject) {
>+            panel.rebuildViewerList(mpp);
>+            last.value = aEvent.subject;
>+          }
>+
>           panel.updateViewerListSelection(mpp);
>         }
Perhaps this belongs in the other attachment, but I think I would be happier if you duplicated code in each branch i.e.
if (aEvent.target == this.mObjectPanel.viewer) {
  if (this.mLastKnownObjectPanelSubject != aEvent.subject) {
    this.mLastKnownObjectPanelSubject = aEvent.subject;
    this.mObjectPanel.rebuildViewerList(this.mObjectViewerListPopup);
  }
  this.mObjectPanel.updateViewerListSelection(this.mObjectViewerListPopup);
}
[But possibly with the locals left in because they are referenced twice.]

>+          var entries = this.mCachedEntries;
>+          if (this.mSubject != aObject || !this.mCachedEntries) {
Nit: use !entries || instead of || !this.mCachedEntries

>+            entries = this.registry.findViewersForObject(aObject, this.id,
>+                                                         this.mLinkedViewer);
>+            if (this.mCachedEntries != entries) {
This will never be true because findViewersForObject always returns a new array each time. r=me with this fixed.

>           else {
>             this.mCurrentViewer.subject = aObject;
>             this.removeAttribute("disabled");
>+            if (didRebuild) {
>+              // We need to ensure here that the viewer list selection gets
>+              // updated, because otherwise it will only get updated when a
>+              // new viewer loads and calls notifyViewerReady.
>+              this.updateViewerListSelection(this.mListElPopup);
>+            }
[Is it possible to skip the whole block if didRebuild is false?]

        Attachment #498359 -
        Flags: review?(neil) → review+

    
    

    
  
Comment on attachment 498358 [details] [diff] [review]
Use getElementsByAttribute for updateViewerListSelection and stop taking entries as a parameter in rebuildViewerList

>-          if ("location" in aEvent.subject) {
>-            this.locationText = aEvent.subject.location; // display document url
...
>+            if ("location" in aEvent.subject) {
>+              // display document url
>+              this.locationText = aEvent.subject.location;
...
>-          }
>-          else if ((nsIAccessibleApplication &&
>-                    aEvent.subject instanceof nsIAccessibleApplication) ||
>-                   (aEvent.subject instanceof nsIAccessible &&
>-                    !aEvent.subject.parent)) {
>+          if ((nsIAccessibleApplication &&
>+               aEvent.subject instanceof nsIAccessibleApplication) ||
>+              (aEvent.subject instanceof nsIAccessible &&
>+               !aEvent.subject.parent)) {
Something went wrong here. The accessible application test seems to have got disconnected from the location test.

>+    if (mpp == this.mDocViewerListPopup) {
>+      panel = this.mDocPanel;
>+    }
>+    else if (mpp == this.mObjectViewerListPopup) {
>+      panel = this.mObjectPanel;
>+    }
>+
>+    if (panel) {
>+      panel.onViewerListCommand(aItem);
>+    }
I'd just write this as:
if (mpp == this.mDocViewerListPopup) {
  this.mDocPanel.onViewerListCommand(aItem);
}
else if (mpp == this.mObjectViewerListPopup) {
  this.mObjectPanel.onViewerListCommand(aItem);
}

>+          return entries;
Don't need to do this any more, since we can use mCachedEntries instead.

    
    

    
  
(In reply to comment #15)
> (In reply to comment #8)
> > (From update of attachment 497174 [details] [diff] [review])
> > I thought the whole point of this was that 1.9.x doesn't have that interface?
> It doesn't, but I don't get any errors about it.
Strange, 1.9.1 doens't warn about "const nsIFoo = Components.interfaces.nsIFoo" but 2.0 does, so this will warn on accessibility-disabled trunk builds. Could we just use "if (Components.interfaces.nsIAccessibleApplication && instead"?

    
    

    
  
How about just leaving it as is then?  I.e., no changes to that area of code, just phase 1, 2, and 3 patches?

    
    

    
  
Comment on attachment 497174 [details] [diff] [review]
cleaunp

Fine by me.

        Attachment #497174 -
        Flags: review?(neil) → review-

    
    

    
  
Comment on attachment 498358 [details] [diff] [review]
Use getElementsByAttribute for updateViewerListSelection and stop taking entries as a parameter in rebuildViewerList

r- for two unrelated nsIApplicationAccessible reasons ;-)

        Attachment #498358 -
        Flags: review?(neil) → review-

    
    

    
  


  

        Attachment #498358 -
        Attachment is obsolete: true

        Attachment #499572 -
        Flags: review?(neil)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        phase 2, address comment 24 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #24)
> >+            entries = this.registry.findViewersForObject(aObject, this.id,
> >+                                                         this.mLinkedViewer);
> >+            if (this.mCachedEntries != entries) {
> This will never be true because findViewersForObject always returns a new array
> each time. r=me with this fixed.

Yeah.  I don't know what I was thinking there.

I'm rerequesting review on this, because I think by "fixed" you meant to get rid of the conditional, and did something else.

Objects having the same entries happens pretty frequently.  E.g., consider looking at multiple menuitems in a popup.  Even in other cases (i.e., not two from a list of like elements), for most elements, the valid viewers will be DOM Node, Box Model, CSS Rules, Computed Style, and JavaScript Object.  In XUL the only thing that really varies is the applicability of the XBL Bindings viewer.  Checking for the entries being the same can eliminate a lot of rebuilding.

This only happens for the panel binding's rebuilds.  The code in inspector.js doesn't check for this shortcut.

        Attachment #498359 -
        Attachment is obsolete: true

        Attachment #499577 -
        Flags: review?(neil)

    
    

    
  
I'm also seeing a problem:

1. Open a new DOM Inspector window and inspect a document
2. Switch to the Style Sheets viewer from the menubar
3. Switch back to the DOM Nodes viewer from the menubar
4. Check the document panel's viewer list.

The Style Sheets viewer is still checked.

When I step through with Venkman, after the checked attribute gets set in updateViewerListSelection, if you inspect the menuitems in another DOM Inspector instance, both the DOM Nodes menuitem and the Style Sheets menuitem have checked="true".  When the menupopup opens, the checked attribute from the DOM Nodes menuitem disappears.

Known bug?

    
    

    
  
(In reply to comment #32)
> When I step through with Venkman, after the checked attribute gets set in
> updateViewerListSelection, if you inspect the menuitems in another DOM
> Inspector instance, both the DOM Nodes menuitem and the Style Sheets menuitem
> have checked="true".  When the menupopup opens, the checked attribute from the
> DOM Nodes menuitem disappears.
> 
> Known bug?
So, what's happening here is that until you open the view menu for the first time, the viewer list has no frame to manage the checked attribute. So the checked state simply builds up. When you open the menu it finally updates, but it doesn't know which one is right, so just picks presumably the last one.

There are two approaches here. The first is to just update the checked state when opening the menu. (The frame always exists during the popupshowing event, so this is never a problem.) The second option is to rebuild the whole menu during the popupshowing event, rather than when the subject changes. (Since you're cacheing the entries on the viewer, this might turn out to be easy.)

    
    

    
  
Comment on attachment 499577 [details] [diff] [review]
phase 2, address comment 24

>+            if (!this.mCachedEntries ||
>+                this.mCachedEntries.toString() != entries.toString()) {
[I can think of a couple of workarounds for the null check, such as making mCachedEntries default to [], or using String(this.mCachedEntries) instead]

        Attachment #499577 -
        Flags: review?(neil) → review+

    
    

    
  
Comment on attachment 499577 [details] [diff] [review]
phase 2, address comment 24

>+          var entries = this.mCachedEntries;
>+          if (this.mSubject != aObject || !entries) {
[Forget that, you do need it to default to null.]

    
  

        Attachment #499572 -
        Flags: review?(neil) → review+

    
    

    
  
I can't actually reproduce that menu bug, but maybe it's OS-dependent.

    
    

    
  
(In reply to comment #33)
> There are two approaches here. The first is to just update the checked state
> when opening the menu. (The frame always exists during the popupshowing event,
> so this is never a problem.) The second option is to rebuild the whole menu
> during the popupshowing event, rather than when the subject changes. (Since
> you're cacheing the entries on the viewer, this might turn out to be easy.)

How about just changing updateViewerListSelection to make sure the previously checked items don't stay checked?

Do you want to review that iteration?

    
    

    
  


  
Buh.  It's easy enough to just attach the patch.

        Attachment #499577 -
        Attachment is obsolete: true

        Attachment #499699 -
        Flags: review?(neil)

    
    

    
  
Comment on attachment 499699 [details] [diff] [review]
phase 2, uncheck previously checked items in updateViewerListSelection

>+          // Uncheck any previously checked items.
>+          var checked = aPopup.getElementsByAttribute("checked", "true");
>+          for (let i = 0, n = checked.length; i < n; ++i) {
>+            checked[i].removeAttribute("checked");
>+          }
NodeLists are live, so this only works because there's never more than one checked element. So perhaps better would be to use the same code style to uncheck the possibly checked item that you do to check the new item.

        Attachment #499699 -
        Flags: review?(neil) → review-

    
    

    
  
Comment on attachment 499743 [details] [diff] [review]
phase 2, uncheck previously checked items, handling unexpected occurrences better

Better?  Even though it's never expected for checked.length to be greater than 1, we can still handle it (for unchecking previously checked items).  The reason it differs from the way checked.length greater than 1 is handled afterward is because in that situation, there's no good way to handle it.

        Attachment #499743 -
        Attachment description: phase 2, uncheck previously checked items, handling cases of → phase 2, uncheck previously checked items, handling unexpected occurrences better

        Attachment #499743 -
        Flags: review?(neil)

    
  

        Attachment #499743 -
        Flags: review?(neil) → review+

        Attachment #499699 -
        Attachment is obsolete: true

        Attachment #497174 -
        Attachment is obsolete: true

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: