Last Comment Bug 765729 - New Tab page thumbnails can't be tabbed to
: New Tab page thumbnails can't be tabbed to
Status: RESOLVED FIXED
[fixed-in-fx-team]
: access
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- major (vote)
: Firefox 16
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on:
Blocks: 455553 719035
  Show dependency treegraph
 
Reported: 2012-06-18 07:36 PDT by Dão Gottwald [:dao]
Modified: 2015-10-11 13:20 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [bugday-20151007]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (856 bytes, patch)
2012-06-21 16:35 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Patch v2 (2.20 KB, patch)
2012-06-22 14:15 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch v3 (2.66 KB, patch)
2012-06-25 11:24 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch v4 (2.45 KB, patch)
2012-06-25 13:05 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-06-18 07:36:44 PDT
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] 2012-06-21 16:35:00 PDT
Created attachment 635525 [details] [diff] [review]
Patch v1
Comment 2 Andres Hernandez [:andreshm] 2012-06-22 14:15:02 PDT
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.
Comment 3 Tim Taubert [:ttaubert] 2012-06-23 07:24:33 PDT
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] 2012-06-25 11:24:27 PDT
Created attachment 636402 [details] [diff] [review]
Patch v3

Applied changes.
Comment 5 Tim Taubert [:ttaubert] 2012-06-25 11:43:34 PDT
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] 2012-06-25 13:05:09 PDT
Created attachment 636459 [details] [diff] [review]
Patch v4
Comment 7 Tim Taubert [:ttaubert] 2012-06-25 13:42:10 PDT
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] 2012-06-25 13:45:54 PDT
https://hg.mozilla.org/integration/fx-team/rev/85d44a26763c
Comment 9 Tim Taubert [:ttaubert] 2012-06-26 11:05:56 PDT
https://hg.mozilla.org/mozilla-central/rev/85d44a26763c
Comment 10 Mohammad Maruf Islam 2015-10-11 13:20:56 PDT
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

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