136291 2014-08-27 10:02:40 -0700 Style's "altText" field (webkit-alt property) is not properly managed. 2025-03-31 22:48:10 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=62774 InRadar P2 Normal --- 133359 1 jfernandez webkit-unassigned ahmad.saleem792 cfleizach darin jdiggs jfernandez karlcow ntim syoichi webkit-bug-importer oldest_to_newest 1031675 0 jfernandez 2014-08-27 10:02:40 -0700 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. 1031677 1 webkit-bug-importer 2014-08-27 10:03:04 -0700 <rdar://problem/18149238> 1031683 2 237230 jfernandez 2014-08-27 10:14:11 -0700 Created attachment 237230 Proposed patch. 1031686 3 jfernandez 2014-08-27 10:18:15 -0700 Chris, since the -webkit-alt property was originally added by your patch for bug #120188 , I'd appreciate your opinion on this issue. Thanks. 1031687 4 237230 cfleizach 2014-08-27 10:19:38 -0700 Comment on attachment 237230 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! 1031696 5 237230 darin 2014-08-27 11:23:24 -0700 Comment on attachment 237230 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. 1031697 6 darin 2014-08-27 11:24:02 -0700 (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! 1031698 7 darin 2014-08-27 11:25:10 -0700 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. 1031710 8 cfleizach 2014-08-27 11:54:13 -0700 (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 1031739 9 jfernandez 2014-08-27 12:42:51 -0700 (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. 237230 2014-08-27 10:14:11 -0700 2014-08-27 11:23:24 -0700 Proposed patch. WIP.patch text/plain 1846 jfernandez RnJvbSA2ZjZmNTZjMTEyMjNlM2YxYzcwODZjZjZiZjlkNjM1NjU2MTkzYmZiIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYXZpZXIgRmVybmFuZGV6IDxqZmVybmFuZGV6QGlnYWxpYS5j b20+CkRhdGU6IFdlZCwgMjcgQXVnIDIwMTQgMTk6MTA6NDkgKzAyMDAKU3ViamVjdDogW1BBVENI XSBQcmVsaW1pbmFyeSBhcHByb2FjaC4KCi0tLQogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3N0 eWxlL0NvbnRlbnREYXRhLmggICAgICAgICAgICAgICAgIHwgNCArKystCiBTb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvc3R5bGUvU3R5bGVSYXJlTm9uSW5oZXJpdGVkRGF0YS5jcHAgfCAxICsKIDIg ZmlsZXMgY2hhbmdlZCwgNCBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3N0eWxlL0NvbnRlbnREYXRhLmggYi9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvQ29udGVudERhdGEuaAppbmRleCBjMDY1Y2JlLi5jMjM3 ODAyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvQ29udGVudERh dGEuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvQ29udGVudERhdGEuaApA QCAtMTAxLDcgKzEwMSw5IEBAIHB1YmxpYzoKIHByaXZhdGU6CiAgICAgdmlydHVhbCBzdGQ6OnVu aXF1ZV9wdHI8Q29udGVudERhdGE+IGNsb25lSW50ZXJuYWwoKSBjb25zdCBvdmVycmlkZQogICAg IHsKLSAgICAgICAgcmV0dXJuIHN0ZDo6bWFrZV91bmlxdWU8SW1hZ2VDb250ZW50RGF0YT4obV9p bWFnZS5nZXQoKSk7CisgICAgICAgIHN0ZDo6dW5pcXVlX3B0cjxDb250ZW50RGF0YT4gaW1hZ2Ug PSBzdGQ6Om1ha2VfdW5pcXVlPEltYWdlQ29udGVudERhdGE+KG1faW1hZ2UuZ2V0KCkpOworICAg ICAgICBpbWFnZS0+c2V0QWx0VGV4dChhbHRUZXh0KCkpOworICAgICAgICByZXR1cm4gaW1hZ2U7 CiAgICAgfQogCiAgICAgUmVmUHRyPFN0eWxlSW1hZ2U+IG1faW1hZ2U7CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvU3R5bGVSYXJlTm9uSW5oZXJpdGVkRGF0YS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3R5bGUvU3R5bGVSYXJlTm9uSW5oZXJpdGVk RGF0YS5jcHAKaW5kZXggZWU4NGFlMi4uYzkwYTVjMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL3N0eWxlL1N0eWxlUmFyZU5vbkluaGVyaXRlZERhdGEuY3BwCisrKyBiL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdHlsZS9TdHlsZVJhcmVOb25Jbmhlcml0ZWREYXRhLmNw cApAQCAtMTI5LDYgKzEyOSw3IEBAIGlubGluZSBTdHlsZVJhcmVOb25Jbmhlcml0ZWREYXRhOjpT dHlsZVJhcmVOb25Jbmhlcml0ZWREYXRhKGNvbnN0IFN0eWxlUmFyZU5vbkluCiAjZW5kaWYKICAg ICAsIG1fY29udGVudChvLm1fY29udGVudCA/IG8ubV9jb250ZW50LT5jbG9uZSgpIDogbnVsbHB0 cikKICAgICAsIG1fY291bnRlckRpcmVjdGl2ZXMoby5tX2NvdW50ZXJEaXJlY3RpdmVzID8gY2xv bmUoKm8ubV9jb3VudGVyRGlyZWN0aXZlcykgOiBudWxscHRyKQorICAgICwgbV9hbHRUZXh0KG8u bV9hbHRUZXh0KQogICAgICwgbV9ib3hTaGFkb3coby5tX2JveFNoYWRvdyA/IHN0ZDo6bWFrZV91 bmlxdWU8U2hhZG93RGF0YT4oKm8ubV9ib3hTaGFkb3cpIDogbnVsbHB0cikKICAgICAsIG1fYm94 UmVmbGVjdChvLm1fYm94UmVmbGVjdCkKICAgICAsIG1fYW5pbWF0aW9ucyhvLm1fYW5pbWF0aW9u cyA/IHN0ZDo6bWFrZV91bmlxdWU8QW5pbWF0aW9uTGlzdD4oKm8ubV9hbmltYXRpb25zKSA6IG51 bGxwdHIpCi0tIAoyLjEuMC5yYzEKCg==