Bug 63573 - REGRESSION (r88415): Range slider doesn't have focus appearance.
Summary: REGRESSION (r88415): Range slider doesn't have focus appearance.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Kent Tamura
Keywords: Regression
Depends on:
Blocks: 63574
  Show dependency treegraph
Reported: 2011-06-28 15:40 PDT by Alexey Proskuryakov
Modified: 2011-07-05 22:30 PDT (History)
2 users (show)

See Also:

slider (223 bytes, text/html)
2011-06-28 15:40 PDT, Alexey Proskuryakov
no flags Details
Patch (1.82 KB, patch)
2011-06-29 19:26 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (30.72 KB, patch)
2011-06-29 19:36 PDT, Kent Tamura
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-06-28 15:40:32 PDT
Created attachment 98982 [details]

Steps to reproduce:
1. Open attached slider.html in Safari.
2. Assuming default user preferences, press Option+Tab several times.

Expected behavior: it should be focused.
Actual behavior: it's not.

Unfortunately, this is the smallest range of revisions I can make. Does someone remember any suspicious changes at that time?
Comment 1 Kent Tamura 2011-06-28 17:09:45 PDT
My changes for tyep=range:

r88294 r=kling [Bug 62207] [Qt] RenderThemeQt::adjustSliderThumbSize() should not refer to the parent style
r88301 r=rniwa [Bug 62195] Convert fast/forms/thumbslider-crash.html to dumpAsText
r88405 r=dglazkov [Bug 62208] Change the argument of RenderTheme::adjustSliderThumbSize(): RenderObject* -> RenderStyle*
r88415 r=dglazkov [Bug 62196] Allow drawing a slider thumb for any nodes

Some focus-related changes in the range:
http://trac.webkit.org/changeset/88418 Makes sure that document.activeElement won't be an element in shadow root.
http://trac.webkit.org/changeset/88421 A forward/backward tab traversal now visits focusable elements in a shadow root.

r88415 and r88421 are suspicious.  I'll investigate.
Comment 2 Kent Tamura 2011-06-29 03:37:38 PDT
(In reply to comment #0)
> Expected behavior: it should be focused.
> Actual behavior: it's not.

Actually it has focus, but has no focus appearance, doesn't it?
Keyboard operation is very slow in this test case because of a wide value range and the default step 1.
Comment 3 Alexey Proskuryakov 2011-06-29 09:01:27 PDT
Looks like that!
Comment 4 Kent Tamura 2011-06-29 19:26:44 PDT
Created attachment 99213 [details]
Comment 5 Kent Tamura 2011-06-29 19:29:04 PDT
Comment on attachment 99213 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=99213&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. This behavior strongly depends on the local computer setting.

Oops, no.  We can make a slider focus regardless of the setting.
Comment 6 Kent Tamura 2011-06-29 19:36:12 PDT
Created attachment 99217 [details]
Patch 2
Comment 7 Alexey Proskuryakov 2011-07-05 12:53:59 PDT
Comment on attachment 99217 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=99217&action=review

> LayoutTests/fast/forms/input-appearance-range.html:7
>     - three horizontal sliders with various thumb positions.
> +     - The second slider has focus.
>     - a vertical slider

Indentation looks strange here.

> Source/WebCore/rendering/RenderThemeMac.mm:1321
> +    updateFocusedState(sliderThumbCell, o->node() && o->node()->focusDelegate()->renderer() ? o->node()->focusDelegate()->renderer() : o);

Can you put parentheses around the ternary operator condition for clarity? Some compilers might actually produce a warning otherwise.
Comment 8 Kent Tamura 2011-07-05 16:34:25 PDT
(In reply to comment #7)

Thank you for reviewing! I'll land with fixes you pointed out.
Comment 9 Kent Tamura 2011-07-05 22:30:04 PDT
Committed r90436: <http://trac.webkit.org/changeset/90436>