Closed Bug 765729 Opened 9 years ago Closed 9 years ago

New Tab page thumbnails can't be tabbed to

Categories

(Firefox :: General, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: dao, Assigned: andreshm)

References

Details

(Keywords: access, Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 3 obsolete files)

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: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #635525 - Flags: review?(ttaubert)
Attached patch Patch v2 (obsolete) — Splinter Review
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)
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+
Attached patch Patch v3 (obsolete) — Splinter Review
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+
Attached patch Patch v4Splinter Review
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
Closed: 9 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]
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]
You need to log in before you can comment on or make changes to this bug.