diff --git a/CHANGELOG.md b/CHANGELOG.md
index ad231060..a58e87a8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,14 @@
# Changelog
+## Unreleased
+
+- Fixed `Attribute::value_source_location()` returning a bogus span for attributes written
+ without a value (e.g. `
`): it now returns `None` for them. Previously it
+ returned a zero-length span pointing at the start of the parser's current buffer — or
+ `None`, depending on where the tag fell relative to a `write()` boundary — and in the
+ `None` case `name_source_location()` was also absent. Attribute source locations no
+ longer depend on how the input was chunked.
+
## v3.0.1
- Improved performance of selector matching on deeply nested elements and on stray end tags.
diff --git a/src/parser/lexer/actions.rs b/src/parser/lexer/actions.rs
index ead181fc..19c26f63 100644
--- a/src/parser/lexer/actions.rs
+++ b/src/parser/lexer/actions.rs
@@ -321,12 +321,14 @@ impl
StateMachineActions for Lexer {
..
}) = self.current_attr
{
- *value = get_token_part_range!(self);
+ let value_range = get_token_part_range!(self);
+
+ *value = Some(value_range);
// NOTE: include closing quote into the raw value if it's present
raw_range.end = match input.get(self.next_pos - 1).copied() {
- Some(ch) if ch == self.closing_quote => value.end + 1,
- _ => value.end,
+ Some(ch) if ch == self.closing_quote => value_range.end + 1,
+ _ => value_range.end,
};
}
}
diff --git a/src/parser/lexer/lexeme/token_outline.rs b/src/parser/lexer/lexeme/token_outline.rs
index 9571e487..727be14f 100644
--- a/src/parser/lexer/lexeme/token_outline.rs
+++ b/src/parser/lexer/lexeme/token_outline.rs
@@ -5,7 +5,8 @@ use crate::parser::AttributeBuffer;
#[derive(Debug, Default, Copy, Clone)]
pub(crate) struct AttributeOutline {
pub name: Range,
- pub value: Range,
+ /// `None` for attributes written without a value (e.g. ``).
+ pub value: Option
,
pub raw_range: Range,
}
diff --git a/src/parser/tree_builder_simulator/mod.rs b/src/parser/tree_builder_simulator/mod.rs
index 07d9dc3e..3f8dfff9 100644
--- a/src/parser/tree_builder_simulator/mod.rs
+++ b/src/parser/tree_builder_simulator/mod.rs
@@ -291,7 +291,9 @@ impl TreeBuilderSimulator {
if !self_closing && eq_case_insensitive(&name, b"annotation-xml") {
for attr in attributes {
let name = lexeme.part(attr.name);
- let value = lexeme.part(attr.value);
+ let Some(value) = lexeme.opt_part(attr.value) else {
+ continue;
+ };
if eq_case_insensitive(&name, b"encoding")
&& (eq_case_insensitive(&value, b"text/html")
diff --git a/src/rewritable_units/tokens/attributes.rs b/src/rewritable_units/tokens/attributes.rs
index cdb3e1d7..1b102c69 100644
--- a/src/rewritable_units/tokens/attributes.rs
+++ b/src/rewritable_units/tokens/attributes.rs
@@ -6,7 +6,6 @@ use crate::rewritable_units::Serialize;
use encoding_rs::Encoding;
use std::cell::OnceCell;
use std::fmt::{self, Debug};
-use std::num::NonZero;
use thiserror::Error;
/// An error that occurs when invalid value is provided for the attribute name.
@@ -42,8 +41,9 @@ pub struct Attribute<'i> {
value: BytesCow<'i>,
raw: Option>,
encoding: &'static Encoding,
- /// absolute document position of attribute name and attribute value
- name_value_start: Option<(usize, NonZero)>,
+ /// Absolute document position of the attribute name and, if the attribute
+ /// was written with a value, of the attribute value.
+ name_value_start: Option<(usize, Option)>,
}
impl<'i> Attribute<'i> {
@@ -54,7 +54,7 @@ impl<'i> Attribute<'i> {
value: BytesCow<'i>,
raw: Bytes<'i>,
encoding: &'static Encoding,
- name_value_start: Option<(usize, NonZero)>,
+ name_value_start: Option<(usize, Option)>,
) -> Self {
Attribute {
name,
@@ -123,12 +123,14 @@ impl<'i> Attribute<'i> {
///
/// The range covers only the value itself, excluding any quotes or the `=` sign.
///
- /// Returns `None` for attributes that were added or modified.
+ /// Returns `None` for attributes that were added or modified, and for attributes
+ /// written without a value (e.g. ``).
#[inline]
#[must_use]
pub fn value_source_location(&self) -> Option
{
self.name_value_start
- .map(|(_, value)| SourceLocation::from_start_len(value.get(), self.value.len()))
+ .and_then(|(_, value)| value)
+ .map(|value| SourceLocation::from_start_len(value, self.value.len()))
}
#[inline]
@@ -279,15 +281,15 @@ impl<'i> Attributes<'i> {
.opt_slice(Some(a.name))
.unwrap_or_else(cant_fail)
.into(),
- self.input
- .opt_slice(Some(a.value))
- .unwrap_or_else(cant_fail)
+ a.value
+ .map(|value| self.input.opt_slice(Some(value)).unwrap_or_else(cant_fail))
+ .unwrap_or_default()
.into(),
self.input
.opt_slice(Some(a.raw_range))
.unwrap_or_else(cant_fail),
self.encoding,
- NonZero::new(base + a.value.start).map(|val| (base + a.name.start, val)),
+ Some((base + a.name.start, a.value.map(|value| base + value.start))),
)
})
}
diff --git a/src/rewriter/mod.rs b/src/rewriter/mod.rs
index dbdf7476..9f4f18cc 100644
--- a/src/rewriter/mod.rs
+++ b/src/rewriter/mod.rs
@@ -1926,6 +1926,90 @@ mod tests {
assert_eq!(&html[locs[2].3.clone().unwrap()], "baz");
}
+ /// Collects `(name span, value span)` for every attribute of every ``,
+ /// feeding the input to the rewriter in `chunk_size`-byte writes.
+ #[allow(clippy::type_complexity)]
+ fn div_attribute_locations(
+ html: &str,
+ chunk_size: usize,
+ ) -> Vec<(
+ Option
>,
+ Option>,
+ )> {
+ let locations = Arc::new(Mutex::new(Vec::new()));
+ let locations_clone = Arc::clone(&locations);
+
+ let mut rewriter = HtmlRewriter::new(
+ Settings::new().append_element_content_handler(element!("div", move |el| {
+ locations_clone
+ .lock()
+ .unwrap()
+ .extend(el.attributes().iter().map(|attr| {
+ (
+ attr.name_source_location().map(|l| l.bytes()),
+ attr.value_source_location().map(|l| l.bytes()),
+ )
+ }));
+ Ok(())
+ })),
+ |_: &[u8]| {},
+ );
+
+ for chunk in html.as_bytes().chunks(chunk_size) {
+ rewriter.write(chunk).unwrap();
+ }
+
+ rewriter.end().unwrap();
+
+ let locations = locations.lock().unwrap();
+
+ locations.clone()
+ }
+
+ // See https://github.com/cloudflare/lol-html/issues/333.
+ #[test]
+ fn attribute_source_locations_for_valueless_attributes() {
+ // NOTE: the comment pushes the tag off byte 0 of the parser's buffer,
+ // which used to turn the bogus value span for `hidden` non-zero.
+ for html in ["", ""] {
+ for chunk_size in [1, 4, 8, 16, html.len()] {
+ let locations = div_attribute_locations(html, chunk_size);
+
+ assert_eq!(locations.len(), 1, "{html:?} at chunk size {chunk_size}");
+
+ let (name, value) = locations[0].clone();
+
+ assert_eq!(
+ &html[name.expect("name should have a source location")],
+ "hidden",
+ "{html:?} at chunk size {chunk_size}"
+ );
+ assert_eq!(
+ value, None,
+ "valueless attribute should have no value source location \
+ ({html:?} at chunk size {chunk_size})"
+ );
+ }
+ }
+ }
+
+ // See https://github.com/cloudflare/lol-html/issues/333.
+ #[test]
+ fn attribute_source_locations_do_not_depend_on_chunk_size() {
+ let html = r#""#;
+ let whole_input = div_attribute_locations(html, html.len());
+
+ assert_eq!(whole_input.len(), 4);
+
+ for chunk_size in [1, 2, 3, 5, 8, 16] {
+ assert_eq!(
+ div_attribute_locations(html, chunk_size),
+ whole_input,
+ "spans changed at chunk size {chunk_size}"
+ );
+ }
+ }
+
#[test]
fn attribute_source_locations_none_for_programmatic_attributes() {
rewrite_str::(
diff --git a/src/selectors_vm/attribute_matcher.rs b/src/selectors_vm/attribute_matcher.rs
index 4ae2f4e5..f9492d57 100644
--- a/src/selectors_vm/attribute_matcher.rs
+++ b/src/selectors_vm/attribute_matcher.rs
@@ -78,8 +78,12 @@ impl<'i> AttributeMatcher<'i> {
#[inline]
fn get_value(&self, lowercased_name: &[u8]) -> Option<&'i [u8]> {
- self.find(lowercased_name)
- .map(|a| self.input.slice(a.value).as_slice())
+ // NOTE: an attribute written without a value has an empty value
+ // as far as selector matching is concerned.
+ self.find(lowercased_name).map(|a| {
+ a.value
+ .map_or(&[][..], |value| self.input.slice(value).as_slice())
+ })
}
#[inline]