Closed Bug 532355 Opened 15 years ago Closed 14 years ago

Implement 'Blink selected element' for accessibleTree view

Categories

(Other Applications :: DOM Inspector, defect)

Product:
Other Applications
Component:
DOM Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(3 files, 3 obsolete files)

patch4
32.48 KB, patch
sdwilsh
: review+
neil
: superreview+
Details | Diff | Splinter Review
colbypatch
4.08 KB, patch
sdwilsh
: review+
neil
: superreview+
Details | Diff | Splinter Review
prefutils patch
3.59 KB, patch
sdwilsh
: review+
neil
: superreview+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
      No description provided.
Attachment #415594 - Flags: superreview?(neil)
Attachment #415594 - Flags: review?(sdwilsh)
(I haven't tried the patch, but I do use the Blink Selected Element menuitem.)
Is bug at all related to bug 368608? (I'm not necessarily saying it is, but I just thought I'd check.)
(In reply to comment #2)
> Is bug at all related to bug 368608? (I'm not necessarily saying it is, but I
> just thought I'd check.)

Only if not explicitly I think. This bug is about to use the flasher for other document views. It changes neither JS Flasher object nor inIFlasher XPCOM object.
(In reply to comment #1)
> (but I do use the Blink Selected Element menuitem.)

Me too, and I wanted to have it working for accessibleTree view, at least partially (because the current approach makes to blink the DOM node of the accessible what is not correct in general, however it works in 90% cases). But I tend to leave this issue for bug 368608.
(In reply to comment #1)
> (I haven't tried the patch, but I do use the Blink Selected Element menuitem.)
I confused the two menuitems. I was thinking of the Blink Element menuitem.

Nit: line 111 (just before observe:) isn't as blank as it should be ;-)
Comment on attachment 415594 [details] [diff] [review]
patch

>       <method name="onViewerChange">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>+          // Disable all commands for for the old viewer.
>           if (aEvent.oldViewer)
>-            // disable all commands for for the old viewer
>             this.setViewerCmdAttribute(aEvent.oldViewer.uid,
>                                        "disabled", "true");
> 
>+          // Enable all commands for for the new viewer.
>           if (aEvent.newViewer)
>-            // enable all commands for for the new viewer
>             this.setViewerCmdAttribute(aEvent.newViewer.uid,
>                                        "disabled", "false");
It seems to me that if this could be improved it would work with shared commands. You might have to tweak the way the attributes work though, since currently it only supports a single viewer uid in the viewer attribute. Possibilities:
1. Use class="viewer uid list" (e.g. class="dom accessibleTree") and querySelector to find the commands matching the uid
2. Use uid="true" attributes to indicate that that command is used by the viewer(s) e.g. dom="true" accessibleTree="true"
Or you may be able to think of a better idea.

>+const nsIDOMNode = Components.interfaces.nsIDOMNode;
We're a little inconsistent here; some code just uses Node.ELEMENT_NODE directly, which comes to the same thing. (In fact instanceof Node happens to work too, but the previous module owner wasn't keen on it.)

>   onPrefChanged: function onPrefChanged(aName)
>   {
>     var value = PrefUtils.getPref(aName);
> 
>     if (aName == "inspector.dom.showAnon") {
>       this.setAnonContent(value);
>+      this.rebuild();
You could use } else { return; } to avoid repeating the rebuild call.
Or it would probably be more readable to convert this to a switch.

>+  <!ENTITY cmdFlashOnSelect.label "Blink Selected Element">
>+  <!ENTITY cmdFlashOnSelect.accesskey "S">
>-
>-  <!ENTITY cmdFlashSelected.label "Blink Selected Element">
>-  <!ENTITY cmdFlashSelected.accesskey "S">
I wonder whether we can mechanically move these in the other locales.
(In reply to comment #5)
> (In reply to comment #1)
> > (I haven't tried the patch, but I do use the Blink Selected Element menuitem.)
> I confused the two menuitems. I was thinking of the Blink Element menuitem.

'Blink element' context menu item is also great thing. But I need to think of an ability to share commands and/or menuitem of context menu, otherwise I need to copy/paste the code. If you like I can move to any way.
(In reply to comment #6)

> It seems to me that if this could be improved it would work with shared
> commands. You might have to tweak the way the attributes work though, since
> currently it only supports a single viewer uid in the viewer attribute.

Ok. I will.

> 
> >+const nsIDOMNode = Components.interfaces.nsIDOMNode;
> We're a little inconsistent here; some code just uses Node.ELEMENT_NODE
> directly, which comes to the same thing. (In fact instanceof Node happens to
> work too, but the previous module owner wasn't keen on it.)

I copied this approach from dom.js. If you really like to change nsIDOMNode to Node usage in accessibleTree.js then I'll do.

> Or it would probably be more readable to convert this to a switch.

I like switch.

> 
> >+  <!ENTITY cmdFlashOnSelect.label "Blink Selected Element">
> >+  <!ENTITY cmdFlashOnSelect.accesskey "S">
> >-
> >-  <!ENTITY cmdFlashSelected.label "Blink Selected Element">
> >-  <!ENTITY cmdFlashSelected.accesskey "S">
> I wonder whether we can mechanically move these in the other locales.

I moved them for updated locales only (ru and sk). I'm not sure it's worth to spend the time for other locales if they were forgotten :) Does it work for you?
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #415594 -
        Attachment is obsolete: true

        Attachment #416009 -
        Flags: superreview?(neil)

        Attachment #416009 -
        Flags: review?(sdwilsh)

        Attachment #415594 -
        Flags: superreview?(neil)

        Attachment #415594 -
        Flags: review?(sdwilsh)

    
    

    
  
Note, if I get right then the latest patch (where Neil's advice from comment #6 is addressed) fixes bug 532312 and bug 517709.

    
    

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

>-          if (aEvent.oldViewer)
>-            // disable all commands for for the old viewer
>-            this.setViewerCmdAttribute(aEvent.oldViewer.uid,
>-                                       "disabled", "true");
>-
>-          if (aEvent.newViewer)
>-            // enable all commands for for the new viewer
>-            this.setViewerCmdAttribute(aEvent.newViewer.uid,
>-                                       "disabled", "false");
>+          // Update commands for the new viewer of the primary panel (Ñurrently
>+          // driven viewers don't have own commands).
>+          if (aEvent.viewerPane.getAttribute("id") == "bxDocPanel")
>+            this.updateViwerCommandsFor(aEvent.newViewer.uid);
This is ugly... I'd prefer if setViewerCmdAttribute only changed the attributes for those commands with a viewer string that contained the uid.

>+      <!-- Enable/disable viwer specific commands for the given view. -->
Viewer, not viwer...

>+          var cmdEls = cmdsetEl.querySelectorAll("command[viewer]");
I'd prefer .getElementsByAtribute("viewer", "*")

>+              var viewerUids = cmdEl.getAttribute("viewer");
>+              if (viewerUids.indexOf(aViwerUid) != -1) {
This doesn't quite work because we have a viewer "domTree" and a viewer "dom"; perhaps you should split the viewerUids string on whitespaces.

    
    

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

will review after comment 11 is addressed.

        Attachment #416009 -
        Flags: review?(sdwilsh)

    
    

    
  
(In reply to comment #11)
> (From update of attachment 416009 [details] [diff] [review])
> >-          if (aEvent.oldViewer)
> >-            // disable all commands for for the old viewer
> >-            this.setViewerCmdAttribute(aEvent.oldViewer.uid,
> >-                                       "disabled", "true");
> >-
> >-          if (aEvent.newViewer)
> >-            // enable all commands for for the new viewer
> >-            this.setViewerCmdAttribute(aEvent.newViewer.uid,
> >-                                       "disabled", "false");
> >+          // Update commands for the new viewer of the primary panel (Ñurrently
> >+          // driven viewers don't have own commands).
> >+          if (aEvent.viewerPane.getAttribute("id") == "bxDocPanel")
> >+            this.updateViwerCommandsFor(aEvent.newViewer.uid);
> This is ugly... I'd prefer if setViewerCmdAttribute only changed the attributes
> for those commands with a viewer string that contained the uid.

Yes, it is. But I don't see a way to change this because the method is called twice. It is called for right panel and then for the left panel. So If I would check uid only then commands for the right panel will be disabled when the method is called second time. Any idea how to change this?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Is it better?

        Attachment #416009 -
        Attachment is obsolete: true

        Attachment #418972 -
        Flags: superreview?(neil)

        Attachment #418972 -
        Flags: review?(sdwilsh)

        Attachment #416009 -
        Flags: superreview?(neil)

    
    

    
  
(In reply to comment #13)
> > This is ugly... I'd prefer if setViewerCmdAttribute only changed the attributes
> > for those commands with a viewer string that contained the uid.
> Yes, it is. But I don't see a way to change this because the method is called
> twice. It is called for right panel and then for the left panel. So If I would
> check uid only then commands for the right panel will be disabled when the
> method is called second time.
No, because you only enable or disable those commands whose viewer attribute contains the uid. For instance, if the command has viewer="dom accessibleTree" then only loading or unloading those two viewers will affect that command.

    
    

    
  
(In reply to comment #15)

> No, because you only enable or disable those commands whose viewer attribute
> contains the uid. For instance, if the command has viewer="dom accessibleTree"
> then only loading or unloading those two viewers will affect that command.

I think I did that in latest patch. It enables commands for new viewer and disables commands for old one.

    
    

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

>+      <method name="updateViwerCommandsFor">
Still needs some s/Viwer/Viewer/g love.

>+          var viewerUidArray = viewerUids.split(/\s/);
>+          return viewerUidArray.some(
>+            function(aElm, aIdx, aArray)
>+            {
>+              return aElm.indexOf(aViwerUid) != -1;
>+            }
>+          );
You're still doing a string indexOf here. You only need to indexOf the array.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch4
        — 
          — Splinter Review
      

    


  

        Attachment #419081 -
        Flags: superreview?

        Attachment #419081 -
        Flags: review?(sdwilsh)

    
  

        Attachment #419081 -
        Flags: superreview? → superreview?(neil)

    
  

        Attachment #418972 -
        Attachment is obsolete: true

        Attachment #418972 -
        Flags: superreview?(neil)

        Attachment #418972 -
        Flags: review?(sdwilsh)

    
    

    
  
(In reply to comment #16)
> I think I did that in latest patch. It enables commands for new viewer and
> disables commands for old one.
Ah, it wasn't so clear in patch 3, but it is clearer in patch 4, thanks.

    
    

    
  
Comment on attachment 419081 [details] [diff] [review]
patch4

>+          // Update commands for the new viewer of the primary panel (Ñurrently
If my UTF8 is correct a Cyrillic character crept in here ;-)

>+            if (aNewViewerUid && viewerUidArray.indexOf(aNewViewerUid) != -1) {
>+              cmdEl.removeAttribute("disabled");
>+              continue;
>+            }
>+
>+            if (aOldViewerUid && viewerUidArray.indexOf(aOldViewerUid) != -1)
>+              cmdEl.setAttribute("disabled", "true");
Using else if instead of continue would also work.

        Attachment #419081 -
        Flags: superreview?(neil) → superreview+

    
    

    
  
(In reply to comment #20)
> (From update of attachment 419081 [details] [diff] [review])
> >+          // Update commands for the new viewer of the primary panel (Ñurrently
> If my UTF8 is correct a Cyrillic character crept in here ;-)

that's quite possible :) thank you for the catch.

> Using else if instead of continue would also work.

ok

    
    

    
  
Comment on attachment 419081 [details] [diff] [review]
patch4

two nits:
application/javascript and not application/x-javascript

please brace even single line if statements

r=sdwilsh

        Attachment #419081 -
        Flags: review?(sdwilsh) → review+

    
    

    
  
(In reply to comment #22)
> please brace even single line if statements

Did this change? (Cf bug 212754, comment 4.)

For what it's worth, I prefer bracing on all if/else and loops, regardless of the number of lines.

    
    

    
  
Yes, it changed.  Trying to bring us inline with https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures even though I dislike K&R bracing style.

    
    

    
  
landed with addressed comments of reviewers - http://hg.mozilla.org/dom-inspector/rev/ff2eefbcbf2d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED

    
    

    
  
maybe, does this cause bustage?

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1262683604.1262683841.16605.gz

cd ../../dist/xpi-stage/venkman && /usr/bin/zip -qr ../venkman-0.9.87.4.xpi *
Comparing fr to en-US
Entities in ./inspector.dtd don't match:
  In /builds/slave/comm-central-trunk-linux/build/mozilla/extensions/inspector/resources/locale/en-US: (add these keys to your localization)
    cmdFlashOnSelect.label
    cmdFlashOnSelect.accesskey

Entities in ./viewers/dom.dtd don't match:
  In /builds/slave/comm-central-trunk-linux/build/mozilla/extensions/inspector/resources/locale/fr: (remove these keys from your localization)
    cmdFlashSelected.label
    cmdFlashSelected.accesskey

NEXT ERROR gmake[9]: *** [libs] Error 1
gmake[9]: Leaving directory `/builds/slave/comm-central-trunk-linux/build/objdir/mozilla/extensions/inspector/resources/locale'
gmake[8]: *** [default] Error 2

    
    

    
  
Thank you for the catch. Eventually fr locale becomes updated and I didn't introduce changes for it. I landed fix - http://hg.mozilla.org/dom-inspector/rev/4290db17526c
Blocks: DOMi2.0.5

    
    

    
  
>+      <desctructor>
>+        this.mFlasher.destroy();
>+        this.mFlasher = null;
>+      </desctructor>

"destructor"

>       <method name="initialize">
>         <body><![CDATA[
>           this.mRegistry = new ViewerRegistry();
>           this.mRegistry.load(kViewerRegURL, this);
>+
>+          this.mFlasher = new DOMIFlasher();
>         ]]></body>
>       </method>

DOMIFlasher isn't noted as a required import in inspector.xml, and isn't
loaded with the "Inspect in New Window" window
(chrome://inspector/content/object.xul):

Error: DOMIFlasher is not defined
Source File: chrome://inspector/content/inspector.xml
Line: 48

    
    

    
  
That should say "Flasher.js isn't noted as a required import..."
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  

      
      
      
      

        Attached patch
          
          
        colbypatch
        — 
          — Splinter Review
      

    


  

        Attachment #420294 -
        Flags: superreview?(neil)

        Attachment #420294 -
        Flags: review?(sdwilsh)

    
    

    
  
>+  <script type="application/x-javascript" src="chrome://inspector/content/Flasher.js"/>

A note: I'm pretty sure r+ from sdwilsh is going to depend on changing this to "application/javascript".

    
    

    
  
Right, thank you. I'll fix this locally.

    
    

    
  
Comment on attachment 420294 [details] [diff] [review]
colbypatch

r=sdwilsh with comment 31 addressed.

        Attachment #420294 -
        Flags: review?(sdwilsh) → review+

    
    

    
  
Comment on attachment 420294 [details] [diff] [review]
colbypatch

[Looks a bit odd just having the one line without the x- or is there a patch somewhere to fix the rest?]

        Attachment #420294 -
        Flags: superreview?(neil) → superreview+

    
    

    
  
(In reply to comment #34)
> (From update of attachment 420294 [details] [diff] [review])
> [Looks a bit odd just having the one line without the x- or is there a patch
> somewhere to fix the rest?]

I don't think so. Do you want me to fix this?

    
    

    
  
I didn't realized Neil isn't cc'ed.

    
    

    
  
Actually I was hoping sdwilsh would answer since he wanted the x- removed.

    
    

    
  
I would be OK with a follow-up.  Over time, I'd like us to remove all x-javascript bits and replace them with just javascript.

    
    

    
  
landed - http://hg.mozilla.org/dom-inspector/rev/b56519458217
Status: REOPENED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED

    
    

    
  
PrefUtils.js also needs to be sourced:

Error: PrefUtils is not defined
Source File: chrome://inspector/content/Flasher.js
Line: 188
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Thanks Colby, I'll fix it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        prefutils patch
        — 
          — Splinter Review
      

    


  

        Attachment #431578 -
        Flags: superreview?(neil)

        Attachment #431578 -
        Flags: review?(sdwilsh)

    
  

        Attachment #431578 -
        Flags: superreview?(neil) → superreview+

    
    

    
  
Comment on attachment 431578 [details] [diff] [review]
prefutils patch

r=sdwilsh

        Attachment #431578 -
        Flags: review?(sdwilsh) → review+

    
    

    
  
landed http://hg.mozilla.org/dom-inspector/rev/4c6aa247ad45
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: