765729 - New Tab page thumbnails can't be tabbed to

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: 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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Applied changes.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 636459 [details] [diff] [review]
Patch v4

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

Thank you, looks good! r=me

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 10

[Mohammad Maruf Islam]

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

Comment 11

[Syed Nayeem Roman]

I have successfully reproduced the bug in firefox Nightly 16.0a1 (2012-06-18)  with Linux x86_64;  

Verified as fixed with latest nightly 56.0a1 (2017-07-23) 

Build ID:   20170722112649 

Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0


[Bugday- 20170719]