Style's "altText" field (webkit-alt property) is not properly managed.

• Component: Accessibility
• Status: NEW ---

[Javier Fernandez]

The recent support of -webkit-alt property for dealing with alternative text in IMG html tags is not properly managed by the RenderStyle (StyleNonInheritedData) and ContentData (ImageContentData) classes. 

As the rest of the fields defined in the RenderStyle class, via the XXXData classes, the constructor using an already instantiated object should copy the field value, in this case the altText and ContentData.altText fields. The bug is not easily reproduced, but its root cause comes from the fact that the StyleNonInheritedData (and the other XXData classes) is defined using the DataRef template, so it uses the access() method to handle its fields. The access() method creates a new instance the first time is called, and a copy reference otherwise. 

So, the bug consists of the altText String pointer being lost when accessing the StyleNonInheritedData fields during the CSS cascade, for instance. This seems to happen, at least during the testing I've done so far, only when setting the webkit-alt string directly; I have not been able to reproduce the issue using the html attribute mechanisms.

This issue seems to affect only the LayoutTests/accessibility/webkit-alt-for-css-content.html tests, which on the other hand is the only one using/testing this new property. 

Finally, I wonder why the mentioned tests is mac specific; I think the test and the property are general enough to be cross platform. So, I'd like to use this bug to define a new cross-platform test.

[Radar WebKit Bug Importer]

<rdar://problem/18149238>

[Javier Fernandez]

Created attachment 237230 [details]
Proposed patch.

[Javier Fernandez]

Chris, since the -webkit-alt property was originally added by your patch for bug #120188 , I'd appreciate your opinion on this issue. Thanks.

[chris fleizach]

Comment on attachment 237230 [details]
Proposed patch.

this looks good to me
Test can be cross platform, should be easy to transition to that. Might be good to include that test change in this patch too

Thanks!

[Darin Adler]

Comment on attachment 237230 [details]
Proposed patch.

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

> Source/WebCore/rendering/style/ContentData.h:104
> +        std::unique_ptr<ContentData> image = std::make_unique<ImageContentData>(m_image.get());

Local variable type should be ImageContentData, not ContentData. I suggest using auto to get that right.

[Darin Adler]

(In reply to comment #0)
> So, I'd like to use this bug to define a new cross-platform test.

I don’t think we need to tie the bug fix to the cross-platform test, but we do want both!

[Darin Adler]

A good test would cover both of the code changes, not just one. In other words, we want a test that would fail if either of the changes would be omitted, possibly two tests.

[chris fleizach]

(In reply to comment #7)
> A good test would cover both of the code changes, not just one. In other words, we want a test that would fail if either of the changes would be omitted, possibly two tests.

Right. We should probably have a new test just to cover this. Then a new bug to make the tests cross platform. Thanks Darin

[Javier Fernandez]

(In reply to comment #8)
> (In reply to comment #7)
> > A good test would cover both of the code changes, not just one. In other words, we want a test that would fail if either of the changes would be omitted, possibly two tests.

Agree.

> 
> Right. We should probably have a new test just to cover this. Then a new bug to make the tests cross platform. Thanks Darin

Filed bug #136300 for that.