Bug 25524 - [Gtk] Expose the title attribute to assistive technologies
Summary: [Gtk] Expose the title attribute to assistive technologies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-03 12:45 PDT by Joanmarie Diggs
Modified: 2009-12-07 11:31 PST (History)
10 users (show)

See Also:


Attachments
test case (607 bytes, text/html)
2009-05-03 12:55 PDT, Joanmarie Diggs
no flags Details
Patch proposal (3.43 KB, patch)
2009-07-17 00:56 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal, with ChangeLog (3.77 KB, patch)
2009-07-17 05:34 PDT, Mario Sanchez Prada
gustavo: review-
Details | Formatted Diff | Diff
Patch proposal, indentation fixed (3.83 KB, patch)
2009-07-17 09:12 PDT, Mario Sanchez Prada
eric: review-
Details | Formatted Diff | Diff
Modified version of Mario's fix: only impacts gtk (4.92 KB, patch)
2009-11-10 20:14 PST, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
Added a new layout test (10.01 KB, patch)
2009-11-22 16:13 PST, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
proposed fix with platform-specific layout test (8.75 KB, patch)
2009-11-24 15:12 PST, Joanmarie Diggs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs 2009-05-03 12:45:37 PDT
According to the W3C Recommendations, the title attribute (http://www.w3.org/TR/html401/struct/global.html#title) "offers advisory information about the element for which it is set."

WebKit-gtk presents this information to the user via a tooltip which appears when the user hovers the mouse over the element in question; however, neither the presence of the title attribute nor its content is exposed as a property of the accessible object to which it pertains.

While "title" suggests that this information might best be exposed as the name of the accessible object, there are a number of reasons why this information might be better suited for exposure through some other means:

1. The W3C Recommendations describe the title attribute as "advisory information," which suggests that both in function and potentially in length, this is not the object's name.

2. The title attribute can be applied to many objects, some of which might already have -- or should have -- accessible names (such as a push button, pending the fix for bug 25523). There needs to be a way to expose both a name and a title, should both be present.

3. Ideally, an assistive technology -- and users thereof -- seeking to obtain the title associated with a particular accessible, should have a reliable, consistent means by which to obtain that information.

Therefore, it is recommended that the content of the title attribute be exposed as the object's accessible description.
Comment 1 Joanmarie Diggs 2009-05-03 12:55:52 PDT
Created attachment 29966 [details]
test case

Here's a test case, should one be of use.

Note that there is an image included with both alternative text and a title attribute. At the moment, WebKit is exposing the alternative text as the accessible description which means it conflicts with my recommendation. :-) A couple of alternatives for the alt text for your consideration:

1. Expose the alt text as the name. This is what Gecko does FWIW.

2. Expose the alt text as the image description (i.e. rather than the accessible's description). Please see http://library.gnome.org/devel/atk/unstable/AtkImage.html

Personally, I could go either way. My thang has always been the consistency/reliability factor: If I know where I can expect to find/access something I'm good. :-) Will, do you have a preference/thoughts on which approach is preferable?
Comment 2 Joanmarie Diggs 2009-05-03 12:58:21 PDT
Sorry for the spam. If there's a way to create a new bug and *at that time* add the attachment *and* set the keyword, please let me know. Thanks!
Comment 3 Mario Sanchez Prada 2009-07-16 14:47:43 PDT
(In reply to comment #0)
> [...]
> Therefore, it is recommended that the content of the title attribute be exposed
> as the object's accessible description.

Trying to reproduce the bug with the test case provided I found out this is now already fixed, so I guess it would be great if you could verify it to make sure this bug is not here anymore :-).

(In reply to comment #1)
> [...]
> Note that there is an image included with both alternative text and a title
> attribute. [...] A couple of alternatives for the alt text for your
> consideration:
> 
> 1. Expose the alt text as the name. This is what Gecko does FWIW.

This is the alternative currently implemented, as far as I could see.

Btw, commit ID where I tried this is 5e4b39b07aa0abb24b5c219cf39d605343368f54, from 2009/07/15.
Comment 4 Joanmarie Diggs 2009-07-16 15:15:16 PDT
(In reply to comment #3)
> (In reply to comment #0)
> > [...]
> > Therefore, it is recommended that the content of the title attribute be exposed
> > as the object's accessible description.
> 
> Trying to reproduce the bug with the test case provided I found out this is now
> already fixed, 

Really??

> so I guess it would be great if you could verify it to make sure
> this bug is not here anymore :-).

When I look in Accerciser at the image, I see the following:

1. The image has no accessible name.

2. The accessible description of this object is "Orca" ("Orca" is the alt text; not the title text.)

3. The title information ("The logo of Orca, screen reader for the GNOME desktop enviornment") is not exposed to ATs from what I'm seeing.

What I expect to see is the following:

1. The image would derive its name from the alt text (i.e. the accessible's name would be "Orca")

2. The accessible description of this object would be "The logo of Orca, screen reader for the GNOME desktop enviornment"

In addition:

1. The "phone number" link has a title attribute ("i.e. the number of your phone :-)"). I do not see that being exposed to ATs. I expect the title attribute to be exposed to ATs -- and done by making that information the accessible description.

2. The entry in which the user is expected to type his/her phone number has a title attribute ("Enter your phone number without any punctuation"). Again, I'm not seeing that information exposed to ATs. (And again, I'd expect it to be exposed via the accessible description.)

3. Ditto for the Submit button (title attribute which is not exposed to ATs, but should be: "If you need a tooltip to tell you to press this button, we don't want your phone number." -- Yeah, I was tired when I made the test case. ;-))

commit 8b7420eab5c08e533279382990d1bc94b9f52360
Date:   Thu Jul 16 21:52:02 2009 +0000
Comment 5 Mario Sanchez Prada 2009-07-16 23:24:28 PDT
(In reply to comment #4)

You're right, I've tried it out now and seen the bug is actually not fixed yet. I guess what happened is that I have probably run 'epiphany' outside my 'jhbuild shell' so it loaded the gecko version (installed in my system) instead the webkit one. That explains why I've seen everything correct in Accerciser :-).

Terrible and silly mistake, I should have double checked that before posting that's already fixed, sorry.
Comment 6 Mario Sanchez Prada 2009-07-17 00:56:06 PDT
Created attachment 32918 [details]
Patch proposal

Exposed 'alt' attribute from images as accessible name and the 'title' attribute from any HTML element as accessible description.
Comment 7 Mario Sanchez Prada 2009-07-17 05:34:01 PDT
Created attachment 32933 [details]
Patch proposal, with ChangeLog

Attaching the patch again, this time with ChangeLog prepared and setting the review:? flag. Sorry for the spam.
Comment 8 Gustavo Noronha (kov) 2009-07-17 07:47:16 PDT
Comment on attachment 32933 [details]
Patch proposal, with ChangeLog

> +2009-07-17  Mario Sanchez Prada  <msanchez@igalia.com>
> +
> +	Reviewed by NOBODY (OOPS!).
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=25524
> +	[Gtk] Expose the title attribute to assistive technologies
> +
> +	Expose 'alt' attribute from images as accessible name.
> +	Expose the 'title' core HTML attribute as accessible description.
> +
> +        * accessibility/AccessibilityRenderObject.cpp:
> +        (WebCore::AccessibilityRenderObject::stringValue):
> +        (WebCore::AccessibilityRenderObject::accessibilityDescription):
> +

The indentation here is completely off. You should align everything at 8 chars, with no tabs at all.

> +        if (node && node->isHTMLElement()) {
> +            const AtomicString& alt = static_cast<HTMLElement*>(node)->getAttribute(altAttr);
> +            if (!alt.isEmpty())
> +              return alt;

Bad indentation here (2 instead of 4 spaces). If you're using Emacs you may want to do M-x set-variable <RET> c-basic-offset <RET> 4 <RET> =)

> +    } else {
> +      Node* node = m_renderer->node();
> +      if (node && node->isHTMLElement()) {
> +        const AtomicString& title = static_cast<HTMLElement*>(node)->getAttribute(titleAttr);
> +        if (!title.isEmpty())
> +          return title;
> +      }

Bad indentation here too.

I could just fix this stuff myself, and commit, since the rest of the patch looks good, but I'd like to have an updated patch submitted by you that I can just apply and commit. Thanks for your work!
Comment 9 Mario Sanchez Prada 2009-07-17 09:12:16 PDT
Created attachment 32944 [details]
Patch proposal, indentation fixed

Fixed indentation problems
Comment 10 Gustavo Noronha (kov) 2009-07-17 09:24:13 PDT
Comment on attachment 32944 [details]
Patch proposal, indentation fixed

r=me
Comment 11 Gustavo Noronha (kov) 2009-07-17 09:49:03 PDT
Landed as r46038.
Comment 12 Eric Seidel (no email) 2009-07-17 13:42:04 PDT
Comment on attachment 32944 [details]
Patch proposal, indentation fixed

This caused regressions:
http://trac.webkit.org/changeset/46038

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r46051%20(2931)/results.html

About to roll this out.
Comment 13 Jan Alonzo 2009-07-17 14:29:02 PDT
(In reply to comment #12)
> (From update of attachment 32944 [details])
> This caused regressions:
> http://trac.webkit.org/changeset/46038
> 
> http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r46051%20(2931)/results.html
> 
> About to roll this out.

Rolled-out in http://trac.webkit.org/changeset/46056

The following tests failed:
accessibility/img-aria-button-alt-tag.html
accessibility/input-image-alt.html
Comment 14 Jan Alonzo 2009-07-17 20:23:48 PDT
Pasting a discussion I've had with Mario so it won't get lost in the ether. Please note that 'img' can be anything. One of the test that failed for example relates to an <img> with a button role so assuming that img as just an img would be inconclusive. We need to check those two tests for the expected behaviours.

-Start----------------------------
10:00 < msanchez> janm: around?
10:02 < janm> msanchez: hey
10:05 < msanchez> hi, just seen the regressions about bug 25524 and I was wondering (not sure though) whether the point there could actually be to change those two failing test cases
10:05 < msanchez> I mean... so far 'alt' was used for accesible descriptions
10:05 < msanchez> but with that implementation, and supposing it's correct (I think that's what gecko does, btw), 'alt' -> accesible name / 'title' -> accesible description
10:06 < msanchez> and those test cases are failing precisely because they expect the 'alt' attribute to be exposed as 'name'...
10:06 < msanchez> what do you think?
10:09 < janm> msanchez: what does the spec say atm? If the spec is not clear, we probably would want to follow FF
10:10 < janm> msanchez: If we need to change the expected results for those two tests, I think we need to engage webkit-dev as well so other ports are aware of the change
10:10 < janm> msanchez: though you might need to do more and update the mac port as well
10:11 < janm> msanchez: (if necessary)
10:11 < msanchez> hmm, don't know, I've just followed indications at https://bugs.webkit.org/show_bug.cgi?id=25524#c1
10:14 < janm> yeah and that's fine. :)
10:14 < janm> msanchez: "In order to be perceivable, elements with a role of img SHOULD have alternative text or a label associated by the accessible name calculation."
10:19 < janm> msanchez: not too sure about if title = accessible description should only apply to imgTags
10:20 < msanchez> janm: actually with that patch it was applying to every element, as it's a core attribute which theoretically should be used for giving extra detail (descriptions) over elements
10:21 < msanchez> http://www.w3schools.com/tags/att_standard_title.asp
10:27 < janm> msanchez: WAI-ARIA seems to suggest that 'title' can be used as the accessible name too http://www.w3.org/TR/wai-aria/
10:31 < msanchez> hmm.. true, perhaps it should be used as a fallback when no other atribute, more suitable for the accesible name, is present
10:32 < janm> msanchez: yeah
10:32 < msanchez> not sure though... I'll keep thinking on that, now I have to go (2.30 am here). It was nice chatting to you, if you come up with a definitive solution to this, it would nice you wrote it down 
                  in the bugzilla :-)
10:34 < msanchez> see you
-End----------------------------
Comment 15 Joanmarie Diggs 2009-07-18 19:36:36 PDT
(In reply to comment #14)

> 10:09 < janm> msanchez: what does the spec say atm? If the spec is not clear,
> we probably would want to follow FF

There are occasions in which FF does some things which make it harder -- not better -- for assistive technologies. IMHO, the way in which they handle images which have both alternative text and a title attribute would be one of those times.

If the spec is not clear, perhaps it would make sense for all interested parties to participate in the writing and/or clarification of the spec needed for implementation.
Comment 16 Mario Sanchez Prada 2009-07-23 02:02:13 PDT
(In reply to comment #15)
> There are occasions in which FF does some things which make it harder -- not
> better -- for assistive technologies. IMHO, the way in which they handle 
> images which have both alternative text and a title attribute would be one 
> of those times.
> 
> If the spec is not clear, perhaps it would make sense for all interested
> parties to participate in the writing and/or clarification of the spec needed
> for implementation.

After reading the section "4.2.7 Accesible Name Calculation" at http://www.w3.org/TR/wai-aria/#nameref, it's my understanding that title should be used only as the last resort when trying to set up the accessible name for a given content:

   A. Elements may specify their text equivalent in DOM attributes, used in 
     this order of preference:
          * [...]
          * [...]
          * Any non-tooltip native markup (such as an img element's alt 
     attribute in HTML), or the subtree for a label pointing to this element 
     (such as label element's for attribute in HTML) contributes according to 
     rules defined by the appropriate specification for that markup.

   B. [...]
   C. [...]
   D. The last resort is to use text from tooltip markup (such as the title 
      attribute in HTML). This is used only if nothing else, including subtree 
      content, has provided results.


However, perhaps I'm missing something here so I dare not to give an strong opinion here :-), but with this information I suppose a good option would be to go ahead with the proposal of:

  * Use 'alt' value for accesible name if apropriate and if it exists. If it does not exist, fallback to 'title' for the accessible name, if present.

  * If the accesible name is already set without using 'title', use this one for the accesible description in case there's not a better candidate for that (aria-label, aria-description).

What do you think? Does this make any sense to you? In case it does, I guess the next thing to do would be, as Jan pointed out, to send a mail to webkit-dev talking about this issue.
Comment 17 Joanmarie Diggs 2009-07-23 06:01:12 PDT
Consider the following:

  <form>
  Phone: <input type="text" title="Enter your number without dashes." />
  </form>

I see stuff like this all of the time. The assumptions made by the content provider presumably are:

1. The user can see (and thus knows "Phone:" is the functional label for the field).

2. The user uses a mouse and will therefore see the the helpful title information as they hover the mouse over the field they are about to fill out.

What a screen reader user wants to hear and/or see in braille is something to the effect of:

  "Phone: text"

Depending on the user, he/she might also want to hear/see: "Enter your number without dashes." (Which is why we need this information exposed.)

Unfortunately, the hypothetical content provider didn't take the time to surround 'Phone:' in <label></label>. So even if bug 25530 were fixed, this text field would have no name.  What a screen reader can do (and what Orca does, and what I believe some of the Windows screen readers do) in this case is attempt to "guess" what the functional label is. For the example above, Orca would guess "Phone:" and present "Phone: text" to the user. So far so good.

Now enter this standard and what I believe you are proposing:

  1. The form field has no name.

  2. The form field has a title.

  3. Ergo, make the accessible name: "Enter your number without dashes."

When a screen reader -- at least Orca -- comes across a form field that has an accessible name, it doesn't try to figure out where that name came from. Nor does it attempt to guess a name/label for a named/labeled field. It assumes the name is valid and presents it. As a result, when a user who is blind Tabs into the above field, what he/she will be presented with is:

  "Enter your number without dashes. text"

Now the user must try to work out what number should be entered without dashes. Phone number is likely, but it might be social security number, or ID number, or order number, or subscriber number depending on the form being filled out. In order to figure out what information is expected, the user must now look around (non-visually) for the functional label. In other words, in an effort to make things more accessible, falling back on the title text makes them less accessible. :-( 

If, instead, the title attribute were exposed as the accessible description, a screen reader like Orca could guess the functional label and present the title information to the user upon request and/or based upon a user-configurable option.

As for images: I feel less strongly about falling back on the title for the accessible name. That said.... If the alt info (if any) is always exposed as the accessible name and the title info (if any) is always exposed as the accessible description, it is quite easy for a screen reader to:

1. Present whatever information happens to be present for the image (i.e. look first to the name and failing that, the description).

2. Implement a setting so that users could opt to hear images with alt text, but skip over images that lack alt text -- even if they have title text.

3. Present detailed "where am I?" information that presents the alt text (or the absence of alt text) and the title text (or the absence of title text) to the user and by doing so helps the user better understand exactly what is on the screen.

In other words, the screen reader is perfectly capable of doing the "fall back to the title as the name" thing. And if you leave it up to the screen reader to do the fallback rather than doing it yourself, the screen reader is able to provide a more consistent experience, customizable to best suit whatever the needs of the user happen to be.
Comment 18 Mario Sanchez Prada 2009-07-23 07:02:40 PDT
(In reply to comment #17)
> [...]
> In other words, the screen reader is perfectly capable of doing the "fall back
> to the title as the name" thing. And if you leave it up to the screen reader to
> do the fallback rather than doing it yourself, the screen reader is able to
> provide a more consistent experience, customizable to best suit whatever the
> needs of the user happen to be.

This makes a lot of sense to me as well, and your examples clarified a lot the situation and why falling back to title for the accessible name is not a good idea at all. Thanks for taking your time for so a good explanation :-)

Therefore, if I understood it well, what you're proposing is going ahead with the original idea implemented in the attached patch: 

  * Always expose 'alt' in images as the accesible name
  * Always expose 'title', for every HTML item, as the accesible description

So, unless I had understood it wrong, this would mean that changing those test cases would be required, as they expect 'alt' to be the accesible description and not the name.

Can you confirm this before I send a mail to webkit-dev proposing this change?
Comment 19 Joanmarie Diggs 2009-07-23 08:00:28 PDT
(In reply to comment #18)
> Therefore, if I understood it well, what you're proposing is going ahead with
> the original idea implemented in the attached patch: 
> 
>   * Always expose 'alt' in images as the accesible name
>   * Always expose 'title', for every HTML item, as the accesible description

That's what I'd like to see done.

> So, unless I had understood it wrong, this would mean that changing those test
> cases would be required, as they expect 'alt' to be the accesible description
> and not the name.

I'm not familiar with the WebKit regression tests or what's being done in terms of WebKitGtk+ accessibility versus platform-independent accessibility, so I'll leave it to someone else to comment.

Thanks!
Comment 20 Mario Sanchez Prada 2009-07-23 08:12:10 PDT
(In reply to comment #19)
> [...]
> That's what I'd like to see done.

Great, so we "just" need to decide about the unit tests then :-)

> [...]
> I'm not familiar with the WebKit regression tests or what's being done in terms
> of WebKitGtk+ accessibility versus platform-independent accessibility, so I'll
> leave it to someone else to comment.

Neither am I... yet :-), just getting started atm, so I agree someone else's opinion on this would be very valuable.

Sending a mail to webkit-dev now then.

Thanks!
Comment 21 Xan Lopez 2009-07-23 10:11:50 PDT
(In reply to comment #19)
> I'm not familiar with the WebKit regression tests or what's being done in terms
> of WebKitGtk+ accessibility versus platform-independent accessibility, so I'll
> leave it to someone else to comment.
> 
> Thanks!

About this particular bit... we want to be able to run all crossplatform tests (when bug #25989 is fixed), and we also have some gtk+ specific tests in place to test our own stuff (for example, I added tests to check that we do the right thing for the ATK get_text_{at,after,before}_offset methods). In this particular case it looks like we want to modify the crossplatform behavior, so we'd need to change the crossplatform tests if we convince everyone else.
Comment 22 Gustavo Noronha (kov) 2009-08-12 18:36:41 PDT
(In reply to comment #21)
> (In reply to comment #19)
> > I'm not familiar with the WebKit regression tests or what's being done in terms
> > of WebKitGtk+ accessibility versus platform-independent accessibility, so I'll
> > leave it to someone else to comment.
> > 
> > Thanks!
> 
> About this particular bit... we want to be able to run all crossplatform tests
> (when bug #25989 is fixed), and we also have some gtk+ specific tests in place
> to test our own stuff (for example, I added tests to check that we do the right
> thing for the ATK get_text_{at,after,before}_offset methods). In this
> particular case it looks like we want to modify the crossplatform behavior, so
> we'd need to change the crossplatform tests if we convince everyone else.

Though I think it would be OK to ifdef this specific case, if we agree to disagree on this, and keep these themes skipped in GTK+, having GTK+-specific tests instead.
Comment 23 Joanmarie Diggs 2009-10-17 19:18:10 PDT
Ping. (Where are we with this?)
Comment 24 Joanmarie Diggs 2009-10-26 14:58:24 PDT
Chris, can we get your opinion on this?

The executive summary is that Mario implemented an awesome solution (thanks!) which I would think would make things better for all platforms. It was committed, but subsequently backed out due to two tests not getting their expected results:

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 32944 [details] [details])
> > This caused regressions:
> > http://trac.webkit.org/changeset/46038
> > 
> > http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r46051%20(2931)/results.html
> > 
> > About to roll this out.
> 
> Rolled-out in http://trac.webkit.org/changeset/46056
> 
> The following tests failed:
> accessibility/img-aria-button-alt-tag.html
> accessibility/input-image-alt.html

Three months have passed, but there's not been any verdict regarding the tests as I understand it.

If need be, I'll see if I can convert Mario's solution into a platform-unique one that doesn't impact any tests. But if you'd think these changes would be of benefit to your users as well, perhaps we can modify the tests.... Or something.... 

Thanks!!
Comment 25 chris fleizach 2009-10-27 23:12:40 PDT
From what I gather, it seems that gtk is not matching the correct data to the right interface.

I think all the pieces are in place already, however and this problem can be fixed on the platform side

String AccessibilityRenderObject::title()

will return titleAttr first if that exists (for all elements)

accessibilityDescription 

will return the altAttr for the image

Right now, the Mac side relies on the fact that axDescription will give us the alt and title will give us the title for the images (which is why the tests are failing)

i think gtk can do the same. First recognize that you're dealing with an image (roleValue==ImageRole), then return the right piece of data
Comment 26 Joanmarie Diggs 2009-11-10 20:14:16 PST
Created attachment 42919 [details]
Modified version of Mario's fix: only impacts gtk

Please review. Thanks!
Comment 27 Jan Alonzo 2009-11-11 03:13:30 PST
(In reply to comment #26)
> Created an attachment (id=42919) [details]
> Modified version of Mario's fix: only impacts gtk
> 
> Please review. Thanks!

Joanie, are you able to run layout tests now? It would be nice if we can cover this with a test or two.
Comment 28 Joanmarie Diggs 2009-11-22 16:13:19 PST
Created attachment 43690 [details]
Added a new layout test

This patch has the same fix as the patch it is obsoleting. What's new is the addition of a layout test.
Comment 29 Xan Lopez 2009-11-24 09:14:01 PST
Comment on attachment 43690 [details]
Added a new layout test

I'm a bit confused about the test: are we skipping in on the other platforms because it does not work atm, but it should? Or are we deviating from them here? If it's the latter it should be a GTK+ only test. From comment #25 it seems that the idea was to unify the behavior. (I haven't beet following closely this bug though, so I might be talking nonsense).
Comment 30 Joanmarie Diggs 2009-11-24 09:23:40 PST
(In reply to comment #29)
> (From update of attachment 43690 [details])
> I'm a bit confused about the test: are we skipping in on the other platforms
> because it does not work atm, but it should? Or are we deviating from them
> here? If it's the latter it should be a GTK+ only test. From comment #25 it
> seems that the idea was to unify the behavior. (I haven't beet following
> closely this bug though, so I might be talking nonsense).

My understanding is that what I expect in terms of behavior and what Chris expects in behavior are different and that, therefore, we needed different tests. But then maybe I'm talking nonsense. :-)

When I looked at the platform-specific tests, the expected results looked like output from DumpRenderTree (rather than the normal expected results).

Please advise, and I will be happy to comply. :-) Thanks!
Comment 31 Xan Lopez 2009-11-24 09:47:53 PST
(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 43690 [details] [details])
> > I'm a bit confused about the test: are we skipping in on the other platforms
> > because it does not work atm, but it should? Or are we deviating from them
> > here? If it's the latter it should be a GTK+ only test. From comment #25 it
> > seems that the idea was to unify the behavior. (I haven't beet following
> > closely this bug though, so I might be talking nonsense).
> 
> My understanding is that what I expect in terms of behavior and what Chris
> expects in behavior are different and that, therefore, we needed different
> tests. But then maybe I'm talking nonsense. :-)

In that case we want GTK only tests, which would go in LayoutTests/platform/gtk

> 
> When I looked at the platform-specific tests, the expected results looked like
> output from DumpRenderTree (rather than the normal expected results).

Hrm, not sure I understand what you mean, but AFAIK it's up to you if tests are dumped as text or if the whole render tree is dumped. You can configure that through the layoutTestController object.

> 
> Please advise, and I will be happy to comply. :-) Thanks!
Comment 32 Gustavo Noronha (kov) 2009-11-24 11:16:33 PST
(In reply to comment #31)

> > When I looked at the platform-specific tests, the expected results looked like
> > output from DumpRenderTree (rather than the normal expected results).
> 
> Hrm, not sure I understand what you mean, but AFAIK it's up to you if tests are
> dumped as text or if the whole render tree is dumped. You can configure that
> through the layoutTestController object.

That's because usually tests are the same for all platforms, but platform differences cause the rendering to not match 100%. This specific case is more akin to LayoutTests/platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html, i.e., the test only makes sense for the platform, as the behavior is specific to it. I would argue that text tests are preferred wherever possible, given that they are not the affected by collateral damage of decisions that change layouting behavior.
Comment 33 Joanmarie Diggs 2009-11-24 15:12:47 PST
Created attachment 43815 [details]
proposed fix with platform-specific layout test

Thanks for the pointers re the test!

Hopefully this is now good to go.
Comment 34 Joanmarie Diggs 2009-11-29 21:10:24 PST
Sorry to be a noodge, but... ping. :-) Thanks!
Comment 35 Adam Barth 2009-11-30 12:38:50 PST
style-queue successfully ran check-webkit-style on attachment 43815 [details] without any errors
Comment 36 Xan Lopez 2009-12-07 05:23:49 PST
Comment on attachment 43815 [details]
proposed fix with platform-specific layout test

LGTM.
Comment 37 WebKit Commit Bot 2009-12-07 05:36:23 PST
Comment on attachment 43815 [details]
proposed fix with platform-specific layout test

Rejecting patch 43815 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Last 500 characters of output:
ut no file of that name could be found
Skipped list contained 'media/media-can-play-ogg.html', but no file of that name could be found
Testing 11733 test cases.
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/http/tests/security)

Exiting early after 1 failures. 8908 tests run.
220.67s total testing time

8907 test cases (99%) succeeded
1 test case (<1%) was new
5 test cases (<1%) had stderr output
Comment 38 Joanmarie Diggs 2009-12-07 05:42:30 PST
I'm assuming that my patch isn't causing the aforementioned test failure. If I'm wrong, I definitely could use a hint.
Comment 39 Xan Lopez 2009-12-07 06:13:37 PST
Comment on attachment 43815 [details]
proposed fix with platform-specific layout test

I don't think it can be related at all, let's try again.
Comment 40 WebKit Commit Bot 2009-12-07 06:27:52 PST
Comment on attachment 43815 [details]
proposed fix with platform-specific layout test

Clearing flags on attachment: 43815

Committed r51762: <http://trac.webkit.org/changeset/51762>
Comment 41 WebKit Commit Bot 2009-12-07 06:28:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Eric Seidel (no email) 2009-12-07 11:31:37 PST
The false rejection was caused by bug 30098.