Support the HTML5 data-* attributes in the sanitizing fragment sink

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({html5})

Trunk
mozilla2.0b7
x86
Mac OS X
html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .11+, status1.9.2 .11-fixed, blocking1.9.1 .14+, status1.9.1 .14-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

HTML5 allows data-* attributes, and we need to allow them in the sanitizing fragment content sink as well.
Created attachment 476901 [details] [diff] [review]
Patch (v1)
Attachment #476901 - Flags: review?(bzbarsky)
Attachment #476901 - Flags: approval2.0?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Comment on attachment 476901 [details] [diff] [review]
Patch (v1)

>+        if (k.Find(NS_LITERAL_STRING("data-")) != 0) {

  if (!StringBeginsWith(k, NS_LITERAL_STRING("data-"))) }

seems more idiomatic.

Also, instead of nested ifs, would it make sense to make use of "&&" here?  At this point we have three-deep nesting for what is fundamentally just an and on several boolean expressions....

r=me with those nits.
Attachment #476901 - Flags: review?(bzbarsky)
Attachment #476901 - Flags: review+
Attachment #476901 - Flags: approval2.0?
Attachment #476901 - Flags: approval2.0+
Blocks: 596300
blocking1.9.1: --- → ?
Created attachment 476907 [details] [diff] [review]
Patch (v1.1)

With comments addressed.
Attachment #476901 - Attachment is obsolete: true

Updated

7 years ago
blocking1.9.1: ? → .14+
blocking1.9.2: ? → .11+
> +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&

That first && should be ||.
Created attachment 476912 [details] [diff] [review]
Patch (v1.2)

(In reply to comment #4)
> > +    if ((!sAllowedAttributes && !sAllowedAttributes->GetEntry(keyAtom)) &&
> 
> That first && should be ||.

Of course!
http://hg.mozilla.org/mozilla-central/rev/fa3912a92409
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Attachment #476912 - Flags: approval1.9.2.11?
Attachment #476912 - Flags: approval1.9.1.14?
Attachment #476907 - Attachment is obsolete: true

Comment 7

7 years ago
Comment on attachment 476912 [details] [diff] [review]
Patch (v1.2)

a=LegNeato for 1.9.2.11 and 1.9.1.14
Attachment #476912 - Flags: approval1.9.2.11?
Attachment #476912 - Flags: approval1.9.2.11+
Attachment #476912 - Flags: approval1.9.1.14?
Attachment #476912 - Flags: approval1.9.1.14+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e69182117a09
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a4d91dc89fc
status1.9.1: --- → .14-fixed
status1.9.2: --- → .11-fixed

Comment 9

7 years ago
This caused the tree to go red:

/builds/slave/mozilla-1.9.2-linux/build/content/html/document/src/nsHTMLFragmentContentSink.cpp:1111: error: no matching function for call to 'StringBeginsWith(nsCAutoString&, const nsString&)'

reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oh, on 1.9.2 and 1.9.1 k is probably a UTF-8 string (and hence you need an NS_LITERAL_CSTRING.

I have no idea why this was reopened, though.  This is fixed on trunk.

I assume you backed it out of 1.9.2, right?  So unsetting that flag.  Did you back out of 1.9.1 as well?
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
status1.9.2: .11-fixed → ---
Resolution: --- → FIXED

Comment 11

7 years ago
Sorry, shouldn't have reopened. I have not backed out the patch as I do not have commit privs. Wasn't sure if we generally file new bugs for bustage or just continue on with the existing one.
Bustage fix pushed on both branches.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dbd650f54b39
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c31c35eeb64e

I chose to use |key| instead of |k|, which is the UTF-16 version of the latter.
status1.9.2: --- → .11-fixed
Christian, for bustage we'd generally use the existing bug, since either a bustage fix is pushed (and then we need to say what the changeset for that was) or the patch is backed out (and then whatever bugzilla bit indicates that the patch is checked in needs to be flipped back to the "not checked in" state).

Updated

6 years ago
blocking2.0: ? → final+
You need to log in before you can comment on or make changes to this bug.