Make FormControlRange containers null and use element-based creation API [chromium/src : main]
Stephanie Zhang (Gerrit)
Message from Stephanie Zhang![]( "Open in Gerrit")
Set Ready For Review
Related details
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephanie Zhang (Gerrit)
Stephanie Zhang added 1 comment![]( "Open in Gerrit")
TEST_F(FormControlRangeTest, InvalidOffsetPreservesState) {
SetBodyContent("<textarea>Hello</textarea>");
auto* textarea = GetDocument().body()->firstElementChild();
auto* range = FormControlRange::Create(GetDocument());
// First set up a valid range.
DummyExceptionStateForTesting valid_exception_state;
range->setFormControlRange(textarea, 1, 4, valid_exception_state);
EXPECT_FALSE(valid_exception_state.HadException());
EXPECT_EQ(range->toString(), "ell");
EXPECT_EQ(range->startOffset(), 1u);
EXPECT_EQ(range->endOffset(), 4u);
// Setting out-of-bounds offsets should fail and preserve previous state.
DummyExceptionStateForTesting invalid_exception_state;
range->setFormControlRange(textarea, 10, 15, invalid_exception_state);
EXPECT_TRUE(invalid_exception_state.HadException());
EXPECT_EQ(invalid_exception_state.CodeAs<DOMExceptionCode>(),
DOMExceptionCode::kIndexSizeError);
EXPECT_EQ(range->toString(), "ell");
EXPECT_EQ(range->startOffset(), 1u);
EXPECT_EQ(range->endOffset(), 4u);
EXPECT_EQ(range->startContainer(), textarea);
EXPECT_EQ(range->endContainer(), textarea);
}
TEST_F(FormControlRangeTest, BackwardsRangesAutoCollapse) {
SetBodyContent("<textarea>Hello</textarea>");
auto* textarea = GetDocument().body()->firstElementChild();
auto* range = FormControlRange::Create(GetDocument());
// Backwards range should auto-collapse to [start, start].
DummyExceptionStateForTesting exception_state;
range->setFormControlRange(textarea, 4, 1, exception_state);
EXPECT_FALSE(exception_state.HadException());
EXPECT_EQ(range->startOffset(), 4u);
EXPECT_EQ(range->endOffset(), 4u);
EXPECT_TRUE(range->collapsed());
EXPECT_EQ(range->toString(), "");
}
class FormControlRangeElementTest
: public FormControlRangeTest,
public testing::WithParamInterface<std::tuple<const char*, bool>> {};
TEST_P(FormControlRangeElementTest, SetFormControlRangeElements) {
const auto& [html, should_be_valid] = GetParam();
SetBodyContent(html);
auto* element = GetDocument().body()->firstElementChild();
auto* range = FormControlRange::Create(GetDocument());
DummyExceptionStateForTesting exception_state;
range->setFormControlRange(element, 0, 4, exception_state);
EXPECT_EQ(exception_state.HadException(), !should_be_valid);
if (should_be_valid) {
EXPECT_FALSE(exception_state.HadException());
EXPECT_EQ(range->startContainer(), element);
EXPECT_EQ(range->endContainer(), element);
EXPECT_EQ(range->startOffset(), 0u);
EXPECT_EQ(range->endOffset(), 4u);
EXPECT_FALSE(range->collapsed());
EXPECT_EQ(range->toString(), "Test");
} else {
EXPECT_EQ(exception_state.CodeAs<DOMExceptionCode>(),
DOMExceptionCode::kNotSupportedError);
}
}
INSTANTIATE_TEST_SUITE_P(
FormControlElements,
FormControlRangeElementTest,
testing::Values(
std::make_tuple("<textarea>Test</textarea>", true),
std::make_tuple("<input type='text' value='Test'>", true),
std::make_tuple("<input type='search' value='Test'>", true),
std::make_tuple("<input type='password' value='Test'>", true),
std::make_tuple("<input type='url' value='Test'>", true),
std::make_tuple("<input type='tel' value='Test'>", true),
std::make_tuple("<div>Test</div>", false),
std::make_tuple("<div contenteditable>Test</div>", false),
std::make_tuple("<input type='checkbox'>", false),
std::make_tuple("<input type='radio'>", false),
std::make_tuple("<input type='submit'>", false),
std::make_tuple("<input type='button'>", false),
std::make_tuple("<select><option>Test</option></select>", false)));
struct OffsetTestCase {
unsigned start_offset;
unsigned end_offset;
bool should_be_valid;
std::optional<const char*> expected_text;
std::optional<unsigned> expected_start;
std::optional<unsigned> expected_end;
std::optional<bool> expected_collapsed;
};
class FormControlRangeOffsetTest
: public FormControlRangeTest,
public testing::WithParamInterface<OffsetTestCase> {};
TEST_P(FormControlRangeOffsetTest, OffsetValidation) {
SetBodyContent("<textarea>Hello</textarea>");
auto* textarea = GetDocument().body()->firstElementChild();
auto* range = FormControlRange::Create(GetDocument());
DummyExceptionStateForTesting exception_state;
range->setFormControlRange(textarea, GetParam().start_offset,
GetParam().end_offset, exception_state);
EXPECT_EQ(exception_state.HadException(), !GetParam().should_be_valid);
if (GetParam().should_be_valid) {
EXPECT_FALSE(exception_state.HadException());
if (GetParam().expected_text.has_value()) {
EXPECT_EQ(range->toString(), GetParam().expected_text.value());
}
if (GetParam().expected_start.has_value()) {
EXPECT_EQ(range->startOffset(), GetParam().expected_start.value());
}
if (GetParam().expected_end.has_value()) {
EXPECT_EQ(range->endOffset(), GetParam().expected_end.value());
}
if (GetParam().expected_collapsed.has_value()) {
EXPECT_EQ(range->collapsed(), GetParam().expected_collapsed.value());
}
} else {
EXPECT_TRUE(exception_state.HadException());
EXPECT_EQ(exception_state.CodeAs<DOMExceptionCode>(),
DOMExceptionCode::kIndexSizeError);
}
}
INSTANTIATE_TEST_SUITE_P(
OffsetCombinations,
FormControlRangeOffsetTest,
testing::Values(
// Valid forward ranges with expected text.
OffsetTestCase{0, 5, true, "Hello", 0, 5, false},
OffsetTestCase{0, 0, true, "", 0, 0, true},
OffsetTestCase{5, 5, true, "", 5, 5, true},
OffsetTestCase{1, 4, true, "ell", 1, 4, false},
OffsetTestCase{0, 1, true, "H", 0, 1, false},
// Backwards ranges that should auto-collapse but not throw.
OffsetTestCase{5, 2, true, "", 5, 5, true},
OffsetTestCase{4, 1, true, "", 4, 4, true},
OffsetTestCase{3, 0, true, "", 3, 3, true},
// Invalid cases - out of bounds.
OffsetTestCase{0, 10, false}, // end > value length.
OffsetTestCase{10, 15, false}, // both offsets > value length.
OffsetTestCase{6, 6, false}, // start at boundary but > value length.
OffsetTestCase{3, 8, false}, // start valid but end > value length.
OffsetTestCase{7, 10, false} // both start and end > value length.
));Deleted tests that were redundant with the wpt tests ([previous CL feedback](https://chromium-review.googlesource.com/c/chromium/src/+/6876353/comment/62bb7551_b5a360f4/))
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephanie Zhang (Gerrit)
Stephanie Zhang added 1 comment![]( "Open in Gerrit")
Related details
- Ana Sollano Kim
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 2 comments![]( "Open in Gerrit")
// TODO: Aggregate text when it spans multiple nodes.Not a change in this patch but do you think it's worth having an open task to track this work?
if (!InputSupportsSelectionAPI()) {Should we also check that the feature is enabled?
Related details
- Dan Clark
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark added 14 comments![]( "Open in Gerrit")
I like this API shape much better. Mostly just had small things/nitpicks.
It'd be good to have a comment here linking to the crbug for this feature that explains why this interface no longer aligns with the spec.
// A live range over a text-control element (<input>, <textarea>) or customnit: I wouldn't say "or custom element" yet, until that's something we actually support.
CHECK(element);This is a subjective nitpick that you are welcome to ignore: I find not-null `CHECK`s in scenarios like this redundant. By using `element` on the next line without a null-check we're already implying that we expect it to never be null, and if it ever were null that line would crash safely. So the `CHECK` doesn't really add anything either semantically or for runtime safety.
// TODO: Aggregate text when it spans multiple nodes.Not a change in this patch but do you think it's worth having an open task to track this work?
Is adding support for non-form elements currently planned, or just something that we're future-proofing the API shape to potentially let us do in the future? If we're not actively planning to work on it, I wouldn't add this TODO at all.
If we're definitely planning to work on it though then I agree with Ana's suggestion to track in a bug/task.
if (!InputSupportsSelectionAPI()) {Should we also check that the feature is enabled?
+1, `CHECK(RuntimeEnabledFeatures::FormControlRangeEnabled())` is reasonable here.
if (!RuntimeEnabledFeatures::FormControlRangeEnabled()) {Instead let's `CHECK(RuntimeEnabledFeatures::FormControlRangeEnabled())`, since the `RuntimeEnabled=FormControlRange` the IDL will protect against this case.
function setupFormControlRange(element, startOffset, endOffset) {Consider getting rid of this helper. It doesn't shorten anything now; the caller can just do `element.getValueRange(startOffset, endOffset)` directly.
function setupFormControlRange(element, startOffset, endOffset){Helper can be removed, caller can just call this directly.
return element.getValueRange(start, end);Helper can be removed, caller can just call this directly.
return element.getValueRange(start, end);Helper can be removed, caller can just call this directly.
return element.getValueRange(start, end);Helper can be removed, caller can just call this directly.
const div = document.createElement('div');This subtest could be improved to check this for all of `HTML5_ELEMENTS` (skipping `<input>`/`<textarea>`).
Alternatively, just delete this subtest; I don't think a test that the method doesn't exist on arbitrary interfaces is particularly useful.
return element.getValueRange(start, end);Helper can be removed, caller can just call this directly.
Related details
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephanie Zhang (Gerrit)
Stephanie Zhang added 13 comments![]( "Open in Gerrit")
It'd be good to have a comment here linking to the crbug for this feature that explains why this interface no longer aligns with the spec.
Done
// A live range over a text-control element (<input>, <textarea>) or customnit: I wouldn't say "or custom element" yet, until that's something we actually support.
Done
This is a subjective nitpick that you are welcome to ignore: I find not-null `CHECK`s in scenarios like this redundant. By using `element` on the next line without a null-check we're already implying that we expect it to never be null, and if it ever were null that line would crash safely. So the `CHECK` doesn't really add anything either semantically or for runtime safety.
Good point - removed.
// TODO: Aggregate text when it spans multiple nodes.Dan ClarkNot a change in this patch but do you think it's worth having an open task to track this work?
Is adding support for non-form elements currently planned, or just something that we're future-proofing the API shape to potentially let us do in the future? If we're not actively planning to work on it, I wouldn't add this TODO at all.
If we're definitely planning to work on it though then I agree with Ana's suggestion to track in a bug/task.
This TODO is about a different issue that Mason flagged in this CL (https://chromium-review.googlesource.com/c/chromium/src/+/7013433/comment/5c8ae0e5_3dca3756/): the inner editor can have multiple text nodes, but we only use the first one for geometry.
Added crbug.com/482337697 to track it.
Dan ClarkShould we also check that the feature is enabled?
+1, `CHECK(RuntimeEnabledFeatures::FormControlRangeEnabled())` is reasonable here.
Good call - done.
if (!RuntimeEnabledFeatures::FormControlRangeEnabled()) {Instead let's `CHECK(RuntimeEnabledFeatures::FormControlRangeEnabled())`, since the `RuntimeEnabled=FormControlRange` the IDL will protect against this case.
Done
function setupFormControlRange(element, startOffset, endOffset) {Consider getting rid of this helper. It doesn't shorten anything now; the caller can just do `element.getValueRange(startOffset, endOffset)` directly.
Done
function setupFormControlRange(element, startOffset, endOffset){Helper can be removed, caller can just call this directly.
Done
Helper can be removed, caller can just call this directly.
Done
Helper can be removed, caller can just call this directly.
Done
Helper can be removed, caller can just call this directly.
Done
This subtest could be improved to check this for all of `HTML5_ELEMENTS` (skipping `<input>`/`<textarea>`).
Alternatively, just delete this subtest; I don't think a test that the method doesn't exist on arbitrary interfaces is particularly useful.
Agreed - deleted the subtest.
Helper can be removed, caller can just call this directly.
- Ana Sollano Kim
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephanie Zhang (Gerrit)
Stephanie Zhang added 1 comment![]( "Open in Gerrit")
- Ana Sollano Kim
- Dan Clark
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dan Clark
| Code-Review | +1 |
2 comments
LGTM
// TODO: Aggregate text when it spans multiple nodes.Dan ClarkNot a change in this patch but do you think it's worth having an open task to track this work?
Stephanie ZhangIs adding support for non-form elements currently planned, or just something that we're future-proofing the API shape to potentially let us do in the future? If we're not actively planning to work on it, I wouldn't add this TODO at all.
If we're definitely planning to work on it though then I agree with Ana's suggestion to track in a bug/task.
This TODO is about a different issue that Mason flagged in this CL (https://chromium-review.googlesource.com/c/chromium/src/+/7013433/comment/5c8ae0e5_3dca3756/): the inner editor can have multiple text nodes, but we only use the first one for geometry.
Added crbug.com/482337697 to track it.
- Ana Sollano Kim
- Mason Freed
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim voted Code-Review+1![]( "Open in Gerrit")
- Mason Freed
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 5 comments![]( "Open in Gerrit")
Looks good to me. But with `setFormControlRange` removed, I think there might be a big tree of code that can also be deleted. Correct me if I'm wrong!
// to hide internal DOM structure.Perhaps add that it only does that when the feature flag is enabled?
[RaisesException, RuntimeEnabled=FormControlRange]
FormControlRange getValueRange(unsigned long start, unsigned long end);Perhaps you make this a mixin? I.e. like this?
// Holds FormControlRange instances that observe this text control.
HeapVector<Member<FormControlRange>> form_control_ranges_;
ditto - still used somewhere? (Other than tests?)
// Register/unregister ranges that need to be notified of value changes.
void RegisterFormControlRange(FormControlRange* range);
void UnregisterFormControlRange(FormControlRange* range);Are these called by anyone now?
Related details
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephanie Zhang (Gerrit)
Stephanie Zhang added 4 comments![]( "Open in Gerrit")
Perhaps add that it only does that when the feature flag is enabled?
Done
[RaisesException, RuntimeEnabled=FormControlRange]
FormControlRange getValueRange(unsigned long start, unsigned long end);Perhaps you make this a mixin? I.e. like this?
Added in `form_control_range.idl`- thanks!
// Holds FormControlRange instances that observe this text control.
HeapVector<Member<FormControlRange>> form_control_ranges_;
ditto - still used somewhere? (Other than tests?)
Since Register/UnregisterFormControlRange() is being kept, it's still needed.
// Register/unregister ranges that need to be notified of value changes.
void RegisterFormControlRange(FormControlRange* range);
void UnregisterFormControlRange(FormControlRange* range);Are these called by anyone now?
RegisterFormControlRange is still being used in the FormControlRange constructor to add the range to the element's set of associated ranges. UnregisterFormControlRange will be used in a follow-up CL implementing element removing steps (still ongoing spec discussions).
Added unregistering/detaching ranges to the CL description as a follow-up CL.
Related details
- Ana Sollano Kim
- Dan Clark
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
// Register/unregister ranges that need to be notified of value changes.
void RegisterFormControlRange(FormControlRange* range);
void UnregisterFormControlRange(FormControlRange* range);Stephanie ZhangAre these called by anyone now?
RegisterFormControlRange is still being used in the FormControlRange constructor to add the range to the element's set of associated ranges. UnregisterFormControlRange will be used in a follow-up CL implementing element removing steps (still ongoing spec discussions).
Added unregistering/detaching ranges to the CL description as a follow-up CL.
- Ana Sollano Kim
- Dan Clark
- Stephanie Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |