616440 - Detail view blocks scrolling with page-up, page-down, and arrow keys

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[B.J. Herbison]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101202 Firefox/4.0b8pre
Build Identifier: 4.0b8pre (installed from nightly on 3 December 2010)

When viewing the description of an installed add-on, the page-up and page-down keys should scroll the displayed description. The keys don't work when you first view a description.

Note: If you press the tab key or any arrow key then the page up and page down keys start working.

Reproducible: Always

Steps to Reproduce:
1.Install LavaFox V1, or other add-on with a long description.
2.Open Add-ons Manager.
3.Click Appearance/Lavafox/More.
4.Read visible text.
5.Press "Page Down" key.
6.Press "Page Down" key several more times in frustration.
7.Check whether your system is hung.
Actual Results:  
Your system is not hung and the first page of the LavaFox description is still visible.

Expected Results:  
Either your system is hung or the last page of the LavaFox description is visible.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's interesting. When you enter the details view all arrow keys don't work either. You will have to select a control in that view first, before scrolling works.

Comment 2

[B.J. Herbison]

(In reply to comment #1)
> That's interesting. When you enter the details view all arrow keys don't work
> either. You will have to select a control in that view first, before scrolling
> works.

Actually, the down arrow key works, but it only has effect the third time it is pressed. The first two times have no visible effect. (Same results on Ubuntu and Windows 7.) I'm going to add another bug report for that behavior.

Comment 3

[Dave Townsend [:mossop]]

Likely we just need to ensure the scrollable area gets focus for this to work.

Comment 6

[Wladimir Palant]

(In reply to Dave Townsend (:Mossop) from comment #3)
> Likely we just need to ensure the scrollable area gets focus for this to
> work.

Yes. As I wrote in the duplicate bug I filed - the keyboard focus stays on the (no longer visible) add-on list. You can press Down a few times and then Enter, it will open the details page for a different add-on.

Comment 7

[custom.firefox.lady [:tawn]]

Confirming info from comment #3 and comment #6. This bug was triggering an issue in my Addons Manager Hilite extension; so I included a fix in version 1.0 (currently on the dev channel). Here's the code I used, in case it's helpful:

var dv = AMdoc.getElementById("detail-view");  //AMdoc is the document obj of the AOM
dv.setAttribute('tabindex', 0); 
dv.focus();

Comment 8

[Angie N.]

I need some help on pin pointing where to correct the bug.  I believe I am in the correct file in ...\mozilla-central\toolkit\mozapps\extensions\content\extensions.js.

I am currently looking at scroll preference (Lines: 2927-29141).  Do I focus on the BoxObject?  I was reading on BoxObject (https://developer.mozilla.org/en/XUL_Tutorial/Box_Objects) and seems feasible but when I add detailViewBoxObject.focus() it does not work.

I may need a little nudge to get me on the right track.

Comment 9

[Wladimir Palant]

You probably want to add |this.currentViewObj.node.focus()| at the end of gViewController.loadViewInternal() - always focus a view when it loads rather than implementing a special case for the details view.

Comment 10

[Rich Gomez]

I am working with Angie on this bug and I tried the suggestion in comment #9 with no luck, do we have to blur the previous view before we focus the current view at the end of the method?

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Rich from comment #10)
> do we have to blur the previous view before we focus the
> current view at the end of the method?

No, just calling .focus() will automatically blur the previously focused element.

However, as comment 7 suggests, you will need to add a tabindex="0" attribute to the detail's view container element (id="details-view") in extensions.xul, which allows that element to gain focus.
See https://developer.mozilla.org/en/Accessibility/Keyboard-navigable_JavaScript_widgets for an explanation on how that works.

You should also add that attribute to the container elements for the other views also (ie, all children of the view-port <deck> element).

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Wladimir Palant from comment #9)
> You probably want to add |this.currentViewObj.node.focus()| at the end of
> gViewController.loadViewInternal() - always focus a view when it loads
> rather than implementing a special case for the details view.

Ideally, the element should be focused *before* the call to .show() (or .refresh()), so that a view's .show() method can change which element gets focus.

Comment 13

[Angie N.]

Attachment: changes made to extensions.js & extensions.xul to focus the add-on description box

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 620576 [details] [diff] [review]
changes made to extensions.js & extensions.xul to focus the add-on description box

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

Thanks - this works good :) Just a couple of things to fix up:

::: toolkit/mozapps/extensions/content/extensions.js
@@ +645,5 @@
>        this.currentViewObj.refresh(view.param, ++this.currentViewRequest, aState);
>      else
>        this.currentViewObj.show(view.param, ++this.currentViewRequest, aState);
> +	  
> +	this.currentViewObj.node.setAttribute('tabindex', 0);  

This isn't needed, since you've already set this attribute for the various elements in extensions.xul

@@ +646,5 @@
>      else
>        this.currentViewObj.show(view.param, ++this.currentViewRequest, aState);
> +	  
> +	this.currentViewObj.node.setAttribute('tabindex', 0);  
> +	this.currentViewObj.node.focus();

Move this up a few lines, so it's directly after the following line:
  this.viewPort.selectedPanel.setAttribute("loading", "true");
(See comment 12 for why I'd like this.)

Also, you've used a tab to indent this line, but we use 2 spaces for indentation.
See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide for general code style hints.

Comment 15

[Rich Gomez]

Attachment: first patch with suggested changes applied

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 621365 [details] [diff] [review]
first patch with suggested changes applied

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

Great - thanks Angie and Rich :)

I'll land this for you within the next couple of days.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/85c4026e625d

Comment 18

[Rob Campbell [:rc] (:robcee)]

you're in mozilla-central!

https://hg.mozilla.org/mozilla-central/rev/85c4026e625d