Bug 33714 – [Qt] QWebSettings attribute for toggle Spatial Navigation on/off

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

This bug is about adding to QWebSettings support to toggle "Spatial Navigation" (SNav) on and off.

SNav itself is being implemented in bug 18662.

[Antonio Gomes]

Created attachment 46674 [details]
patch 0.1

[WebKit Review Bot]

Attachment 46674 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/190174

[Antonio Gomes]

(In reply to comment #2)
> Attachment 46674 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/190174

I will ignore the build error for now, because this patch depends on patch in bug 18662.

[Antonio Gomes]

Created attachment 46698 [details]
patch 0.2

better docs

[WebKit Review Bot]

Attachment 46698 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/190451

[Simon Hausmann]

Comment on attachment 46698 [details]
patch 0.2

Antonio, I think your patch looks fine, but I would prefer a slightly more elaborate explanation in the docs on what the _effect_ of enabling this feature is, i.e. that it affects the keyboard navigation.

[Antonio Gomes]

thx for reviewing, simon. I will improve the patch.

(In reply to comment #6)
> (From update of attachment 46698 [details])
> Antonio, I think your patch looks fine, but I would prefer a slightly more
> elaborate explanation in the docs on what the _effect_ of enabling this feature
> is, i.e. that it affects the keyboard navigation.

@simon: something else that came to mind: as is, the patch will enable/disable SNav per-page, i.e. if two windows are opened at the same time, one can be with SNav ON while it is OFF in other. Is that what we want or we want it to be set a globally  ?

[Antonio Gomes]

Created attachment 46879 [details]
(committed in r55579) patch 0.3

Added a more detailed explanation about effects on enabling/disabling SNav.

[Simon Hausmann]

Comment on attachment 46879 [details]
(committed in r55579) patch 0.3

Looks good to me. Thanks Antonio for the improved docs :)

[Eric Seidel (no email)]

Ping?  This was r+'d two weeks ago.

[Antonio Gomes]

(In reply to comment #10)
> Ping?  This was r+'d two weeks ago.

eric, this patch can land before its blocker (bug 18662), which pends review.

[Antonio Gomes]

(In reply to comment #11)
> (In reply to comment #10)
> > Ping?  This was r+'d two weeks ago.
> 
> eric, this patch can land before its blocker (bug 18662), which pends review.

s/can/can not/gc

sorry for spamming it.

[Antonio Gomes]

Comment on attachment 46879 [details]
(committed in r55579) patch 0.3

Clearing r+/cq- since this patch depends on another bug to be landed first.

so it does not appear on pending-commit page.

[Antonio Gomes]

landed in r55579

thx for reviewing you simon.

[Antonio Gomes]

Comment on attachment 46879 [details]
(committed in r55579) patch 0.3

clearing flags