Skip to content
Permalink
Browse files
data-x-2="" not represented in dataset
https://bugs.webkit.org/show_bug.cgi?id=123890

Reviewed by Brent Fulgham.

The bug was caused by propertyNameMatchesAttributeName implementing comparison incorrectly.

Fixed the bug by eliminating this function and reusing convertAttributeNameToPropertyName
when there is exactly one attribute. While the new code does allocate a String object,
it's still faster than creating a AtomString.

* LayoutTests/fast/dom/dataset-expected.txt:
* LayoutTests/fast/dom/dataset.html: Added a test case.

* Source/WebCore/dom/DatasetDOMStringMap.cpp:
(WebCore::propertyNameMatchesAttributeName): Deleted.
(WebCore::DatasetDOMStringMap::isSupportedPropertyName const):
(WebCore::DatasetDOMStringMap::item const):

Canonical link: https://commits.webkit.org/253625@main
  • Loading branch information
rniwa committed Aug 21, 2022
1 parent 4cae97e commit 8a40482a9c1025c3579f72e2a9eee64c369bd0e6
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 33 deletions.
@@ -13,6 +13,10 @@ PASS testGet('data---foo---bar', '-Foo--Bar') is true
PASS testGet('data-foo-', 'foo-') is true
PASS testGet('data-foo--', 'foo--') is true
PASS testGet('data-Foo', 'foo') is true
PASS testGet('data-x-2', 'x-2') is true
PASS testGet('data-2-x', '2X') is true
PASS testGet('data-2', '2') is true
PASS testGet('data-x3', 'x3') is true
PASS testGet('data-', '') is true
PASS testGet('data-à', 'à') is true
PASS document.body.dataset.nonExisting is undefined.
@@ -24,6 +24,10 @@
shouldBeTrue("testGet('data-foo-', 'foo-')");
shouldBeTrue("testGet('data-foo--', 'foo--')");
shouldBeTrue("testGet('data-Foo', 'foo')"); // HTML lowercases all attributes.
shouldBeTrue("testGet('data-x-2', 'x-2')");
shouldBeTrue("testGet('data-2-x', '2X')");
shouldBeTrue("testGet('data-2', '2')");
shouldBeTrue("testGet('data-x3', 'x3')");
shouldBeTrue("testGet('data-', '')");
shouldBeTrue("testGet('data-\xE0', '\xE0')");
shouldBeUndefined("document.body.dataset.nonExisting");
@@ -71,33 +71,6 @@ static String convertAttributeNameToPropertyName(const String& name)
return stringBuilder.toString();
}

static bool propertyNameMatchesAttributeName(const String& propertyName, const String& attributeName)
{
if (!attributeName.startsWith("data-"_s))
return false;

unsigned propertyLength = propertyName.length();
unsigned attributeLength = attributeName.length();

unsigned a = 5;
unsigned p = 0;
bool wordBoundary = false;
while (a < attributeLength && p < propertyLength) {
const UChar currentAttributeNameChar = attributeName[a];
if (currentAttributeNameChar == '-' && a + 1 < attributeLength && attributeName[a + 1] != '-')
wordBoundary = true;
else {
if ((wordBoundary ? toASCIIUpper(currentAttributeNameChar) : currentAttributeNameChar) != propertyName[p])
return false;
p++;
wordBoundary = false;
}
a++;
}

return (a == attributeLength && p == propertyLength);
}

static bool isValidPropertyName(const String& name)
{
unsigned length = name.length();
@@ -160,10 +133,9 @@ bool DatasetDOMStringMap::isSupportedPropertyName(const String& propertyName) co

auto attributeIteratorAccessor = m_element.attributesIterator();
if (attributeIteratorAccessor.attributeCount() == 1) {
// If the node has a single attribute, it is the dataset member accessed in most cases.
// Building a new AtomString in that case is overkill so we do a direct character comparison.
// Avoid creating AtomString when there is only one attribute.
const auto& attribute = *attributeIteratorAccessor.begin();
if (propertyNameMatchesAttributeName(propertyName, attribute.localName()))
if (convertAttributeNameToPropertyName(attribute.localName()) == propertyName)
return true;
} else {
auto attributeName = convertPropertyNameToAttributeName(propertyName);
@@ -197,10 +169,9 @@ const AtomString* DatasetDOMStringMap::item(const String& propertyName) const
AttributeIteratorAccessor attributeIteratorAccessor = m_element.attributesIterator();

if (attributeIteratorAccessor.attributeCount() == 1) {
// If the node has a single attribute, it is the dataset member accessed in most cases.
// Building a new AtomString in that case is overkill so we do a direct character comparison.
// Avoid creating AtomString when there is only one attribute.
const Attribute& attribute = *attributeIteratorAccessor.begin();
if (propertyNameMatchesAttributeName(propertyName, attribute.localName()))
if (convertAttributeNameToPropertyName(attribute.localName()) == propertyName)
return &attribute.value();
} else {
AtomString attributeName = convertPropertyNameToAttributeName(propertyName);

0 comments on commit 8a40482

Please sign in to comment.