Bug 171803 - [GTK] Type ahead find for <option> elements inside an <optgroup> does not work in popup
Summary: [GTK] Type ahead find for <option> elements inside an <optgroup> does not wor...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on: 171492
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-08 06:03 PDT by Adrian Perez
Modified: 2018-05-31 23:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.89 KB, patch)
2017-05-11 03:08 PDT, Claudio Saavedra
cgarcia: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (1.77 MB, application/zip)
2017-05-11 04:31 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2017-05-08 06:03:36 PDT
If a <select> element contains <option>s inside an <optgroup>, the type-ahead
search never picks those <option>s when the popup menu is opened.

To reproduce the issue, save the following HTML snippet in a file:

  <!DOCTYPE html>
  <html>
    <body>
      <form action="#">
        <label for="opt">Choose</label>
        <select name="opt">
          <optgroup label="Fruits">
            <option>Apple</option>
            <option>Strawberry</option>
          </optgroup>
          <optgroup label="Desserts">
            <option>Cake</option>
            <option>Flan</option>
          </optgroup>
        </select>
      </form>
    </body>
  </html>

Then open it (e.g. in MiniBrowser), click the element to make the popup menu
appear, and try entering the first letter of one of the <option>s. For example
entering “s” should select “Strawberry“, but currently this is not working.
Comment 1 Adrian Perez 2017-05-08 06:07:59 PDT
A couple more notes:

 - This happens with current ”trunk”, and with the latest stable
   release (2.16.1). Most likely this has not ever worked well.

 - Both Firefox and Chromium handle type-ahead search correctly
   when <optgroup>s are used (tested with the snippet above).

 - If the <select> element is focused, but the popup is *not
   opened*, then the type ahead search works (likely this is
   case is handled in the shared WebKit code).
Comment 2 Claudio Saavedra 2017-05-11 03:08:16 PDT
Created attachment 309703 [details]
Patch
Comment 3 Build Bot 2017-05-11 04:31:13 PDT
Comment on attachment 309703 [details]
Patch

Attachment 309703 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3717316

New failing tests:
fast/dom/Window/setTimeout-setInterval-unique.html
Comment 4 Build Bot 2017-05-11 04:31:14 PDT
Created attachment 309705 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Carlos Garcia Campos 2017-05-11 04:39:57 PDT
Comment on attachment 309703 [details]
Patch

Thanks! Could we add a tests for this?
Comment 6 Adrian Perez 2017-05-11 04:51:32 PDT
(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 309703 [details]
> Patch
> 
> Thanks! Could we add a tests for this?

I have been trying to fix bug #171492 (without much luck so far; hint: help
welcome) before trying to post a patch for this one, precisely to be able
of adding the test case :-P

BTW, Claudio's patch looks good to me and works.
Comment 7 Claudio Saavedra 2017-05-11 06:30:48 PDT
(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 309703 [details]
> Patch
> 
> Thanks! Could we add a tests for this?

There is already a test in place, but it seems to be broken (regardless of this patch), see bug 171492.
Comment 8 Carlos Garcia Campos 2017-05-11 06:34:30 PDT
(In reply to Claudio Saavedra from comment #7)
> (In reply to Carlos Garcia Campos from comment #5)
> > Comment on attachment 309703 [details]
> > Patch
> > 
> > Thanks! Could we add a tests for this?
> 
> There is already a test in place, but it seems to be broken (regardless of
> this patch), see bug 171492.

But does that test already check this particular issue?
Comment 9 Claudio Saavedra 2017-05-11 07:11:31 PDT
OK, you're right, it doesn't, because that test doesn't have the <optgroup> so there is no space indentation. I'll cook a test but I am afraid it will fail nonetheless for the same reasons than the other one.
Comment 10 Adrian Perez 2017-06-16 09:28:06 PDT
(In reply to Claudio Saavedra from comment #9)
> OK, you're right, it doesn't, because that test doesn't have the <optgroup>
> so there is no space indentation. I'll cook a test but I am afraid it will
> fail nonetheless for the same reasons than the other one.

We have now the test passing in trunk. Could you take a look at adding
a test now for items inside an <optgroup>, so we can get this fix landed?
(Thanks!)