1374784 - Unable to use keyboard to navigate cookies

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Since bug 1146194 was fixed it has not been possible to use the keyboard to navigate the cookies table.

STR:

1. Open https://addons.mozilla.org/firefox/
2. Press <shift><f9> to open the storage inspector.
3. Double-click "__utma" to enter edit mode.
4. Press <tab>.

Actual:

The cell below the edited cell is focused and the original cell is still in edit mode.

Expected:

The original edit should be closed and the cell to the right of it should be focused and enter edit mode.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

358132:34fce7c12173 mratcliffe Bug 1146194 - Multiple cookies with the same name not shown r=gl

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug 1146194 landed in Firefox 53

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1374784 - Unable to use keyboard to navigate cookies

Keyboard navigation was broken because we introduced private columns that are never displayed (for compound primary keys) and were not equipped to deal with hidden key columns.

This change gets the first visible column, whether it is private or not so that we no longer have to worry about hidden key columns.

The small change to the keyboard test ensures that the private column is hidden so the test will fail if this feature is broken again.

Review commit: https://reviewboard.mozilla.org/r/151316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151316/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8879921 [details]
Bug 1374784 - Unable to use keyboard to navigate cookies

https://reviewboard.mozilla.org/r/151316/#review156450

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea5a5610ff7e
Unable to use keyboard to navigate cookies r=gl

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea5a5610ff7e

Comment 7

[Julien Cristau [:jcristau]]

Not a new regression, fix-optional for 55, though feel free to request uplift after this is verified in nightly.

Comment 8

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 56.0a1 (2017-06-20) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

Build ID   : 20170623115718
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 9

[Tanvir Rahman]

I have reproduced this Bug on Nightly 56.0a1 (2017-06-20) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Nightly

Build ID 	20170623045418
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170621]

Comment 10

[Carsten Book [:Tomcat]]

sorry hat to back this out in https://treeherder.mozilla.org/logviewer.html#?job_id=109864699&repo=mozilla-central since somehow this test is now a perma failure. Mike can you take a look ?

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/da4ac701aae7
Backed out changeset ea5a5610ff7e for getting perma failures are merge

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1374784 - Unable to use keyboard to navigate cookies

Review commit: https://reviewboard.mozilla.org/r/152624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152624/

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8881485 [details]
Bug 1374784 - Unable to use keyboard to navigate cookies

https://reviewboard.mozilla.org/r/151314/#review157992

This patch bounced previously but is completely green on try wouthout any changes.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8881485 [details]
Bug 1374784 - Unable to use keyboard to navigate cookies

https://reviewboard.mozilla.org/r/152624/#review157994

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16ae2d86aae4
Unable to use keyboard to navigate cookies r=miker

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16ae2d86aae4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Did you want to consider uplifting this to Beta so DevEdition users get the fix before next month?

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Did you want to consider uplifting this to Beta so DevEdition users get the
> fix before next month?

Great point, yes, I would like to request uplift but don't see the UI to do so... I assume I need to use the tracking section?

[Tracking Requested - why for this release]:
Fixes keyboard navigation in the storage inspector... this is needed for accessibility.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8881485 [details]
Bug 1374784 - Unable to use keyboard to navigate cookies

Approval Request Comment
[Feature/Bug causing the regression]: Storage inspector rewrite
[User impact if declined]: Storage Inspector will not be usable for keyboard users (accessibility issue).
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It is a fairly simple change
[String changes made/needed]: None

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8881485 [details]
Bug 1374784 - Unable to use keyboard to navigate cookies

devtools fix, beta55+

should be in 55.0b9

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e6ca58f20db

Comment 22

[Andrei Vaida [:avaida]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Michael's assessment on manual testing needs and the fact that this fix has automated coverage.