The default bug view has changed. See this FAQ.

New Tab page thumbnails can't be tabbed to

RESOLVED FIXED in Firefox 16

Status

()

Firefox
General
--
major
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dao, Assigned: andreshm)

Tracking

({access})

Trunk
Firefox 16
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Opening a new tab and pressing TAB again and again won't ever focus the first thumbnail. It seems like focus gets stuck somewhere. You can however use Shift-TAB to reach the last thumbnail.
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 635525 [details] [diff] [review]
Patch v1
Attachment #635525 - Flags: review?(ttaubert)
(Assignee)

Comment 2

5 years ago
Created attachment 635911 [details] [diff] [review]
Patch v2

Patch improvement, to prevent the focus when pressing tab and the new tab page is hidden. Currently, it was disabling only the pointer-events, but the key tab was still focusing the grid's buttons.
Attachment #635525 - Attachment is obsolete: true
Attachment #635525 - Flags: review?(ttaubert)
Attachment #635911 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Blocks: 719035
Comment on attachment 635911 [details] [diff] [review]
Patch v2

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

Thank you for finding a fix for this! Also, making sure that all affected elements are not focusable when the page is disabled is a very good point.

::: browser/base/content/newtab/newTab.css
@@ -3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -:root {
> -  -moz-user-focus: normal;
> -}

We should not remove this as we can't unfocus the url bar then. We should move it to #newtab-scrollbox because that spans the whole page. So we can unfocus the url bar by clicking anywhere on the page and can forward-tab to our thumbnails again.

::: browser/base/content/newtab/page.js
@@ +85,3 @@
>  
>      // Set the nodes' states.
>      for (let i = 0; i < nodes.length; i++) {

for (let node of document.querySelectorAll(selector)) {

@@ +90,5 @@
>          node.removeAttribute("page-disabled");
>        else
>          node.setAttribute("page-disabled", "true");
> +      if (node.id == "newtab-grid")
> +        this._enableInputs(node, aValue);

Please just do this under the for-loop with the selector mentioned below.

@@ +105,5 @@
> +   * @param aValue true if enabled, false otherwise.
> +   */
> +  _enableInputs : function(aNode, aValue) {
> +    let inputs = aNode.querySelectorAll("input");
> +    for (let i = 0; i < inputs.length; i++) {

for (let input of document.querySelectorAll(".newtab-control, .newtab-link")) {

Note the selector, we also need to make sure that the links aren't focused.

@@ +107,5 @@
> +  _enableInputs : function(aNode, aValue) {
> +    let inputs = aNode.querySelectorAll("input");
> +    for (let i = 0; i < inputs.length; i++) {
> +      if (aValue) {
> +        inputs[i].removeAttribute("disabled");

input.removeAttribute("tabindex");

@@ +109,5 @@
> +    for (let i = 0; i < inputs.length; i++) {
> +      if (aValue) {
> +        inputs[i].removeAttribute("disabled");
> +      } else {
> +        inputs[i].setAttribute("disabled", "true");

input.setAttribute("tabindex", "-1");

That will prevent inputs and links from getting focus.
Attachment #635911 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 636402 [details] [diff] [review]
Patch v3

Applied changes.
Attachment #635911 - Attachment is obsolete: true
Attachment #636402 - Flags: review?(ttaubert)
Comment on attachment 636402 [details] [diff] [review]
Patch v3

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

::: browser/base/content/newtab/page.js
@@ +100,5 @@
>    /**
> +   * Enables/disables the new tab's control and link elements.
> +   * @param aValue true if enabled, false otherwise.
> +   */
> +  _enableInputs : function(aValue) {

I'd rather just include this in the _updateAttributes() method. _enableInputs isn't a great method name and so it's all in one place :)
Attachment #636402 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 636459 [details] [diff] [review]
Patch v4
Attachment #636402 - Attachment is obsolete: true
Attachment #636459 - Flags: review?(ttaubert)
Comment on attachment 636459 [details] [diff] [review]
Patch v4

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

Thank you, looks good! r=me
Attachment #636459 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/85d44a26763c
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/85d44a26763c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I have reproduced this bug with Firefox Nightly 16.0a1 (Build ID:20120618030531) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 44.0a1(Build ID: 20151011030229)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
QA Whiteboard: [bugday-20151007]
You need to log in before you can comment on or make changes to this bug.