making image element areas good at finding areas#15904
making image element areas good at finding areas#15904bors-servo merged 1 commit intoservo:masterfrom
Conversation
|
|
highfive
added
the
S-awaiting-review
|
|
||
| let map = self.upcast::<Node>() | ||
| .following_siblings() | ||
| .GetRootNode() |
There was a problem hiding this comment.
Use the document instead.
document_from_node(self).traverse_preorder()| Some(elements) | ||
| Some(elements) | ||
| } | ||
| } |
There was a problem hiding this comment.
This would be shorter as map.map(|elem| { ... }), probably with a different name for map, for clarity.
nox
added
S-needs-code-changes
|
#15914 partly covers the same changes. |
|
|
||
| let elements: Vec<Root<HTMLAreaElement>> = map.unwrap().upcast::<Node>() | ||
| .children() | ||
| match map { |
There was a problem hiding this comment.
map.and_then(|map_elem| {
// elements ...
})There was a problem hiding this comment.
Yeah, got confused by Some returning another Some
sendilkumarn
force-pushed
the
image-area
branch
from
c69e01b to
37ff7ee
Compare
highfive
added
S-awaiting-review
nox
left a comment
nox
left a comment
There was a problem hiding this comment.
One nit, one fix, and one refactor.
| let map = self.upcast::<Node>() | ||
| .following_siblings() | ||
| let useMapElements = self.upcast::<Node>() | ||
| .GetRootNode() |
There was a problem hiding this comment.
Use the document here as I mentioned in my previous comment, also the indentation is wrong.
There was a problem hiding this comment.
want to replace node by document ?
There was a problem hiding this comment.
I don't understand this here.
There was a problem hiding this comment.
Previously nox had mentioned this,
document_from_node(self).traverse_preorder()There was a problem hiding this comment.
Dont we have to cast the doc into node to traverse?
There was a problem hiding this comment.
Yes we do.
document_from_node(self).upcast::<Node>().traverse_preorder()Still better than using GetRootNode.
|
|
||
| let map = self.upcast::<Node>() | ||
| .following_siblings() | ||
| let useMapElements = self.upcast::<Node>() |
| mapElem.upcast::<Node>() | ||
| .traverse_preorder() | ||
| .filter_map(Root::downcast::<HTMLAreaElement>).collect() | ||
| }) |
There was a problem hiding this comment.
Actually I just realised that you should rather add a method on HTMLMapElement itself that returns the Vec<Root<HTMLAreaElement>>, and call it here.
nox
added
S-needs-code-changes
sendilkumarn
force-pushed
the
image-area
branch
from
37ff7ee to
7dbf66f
Compare
highfive
added
S-awaiting-review
sendilkumarn
force-pushed
the
image-area
branch
from
7dbf66f to
7a3644a
Compare
|
@bors-servo try |
making image element areas good at finding areas <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15884 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15904) <!-- Reviewable:end -->
|
💔 Test failed - linux-rel-wpt |
|
The only test that failed that wasn't recognized in that run was an instance of #14323. |
|
@sendilkumarn The code is correct, but it needs a test. |
nox
added
S-needs-tests
|
I am on it. |
|
@nox where / what you want to add in tests? |
sendilkumarn
force-pushed
the
image-area
branch
from
7a3644a to
1d2569c
Compare
highfive
added
the
S-awaiting-review
|
@nox sorry for the delay. Here it is 👍 |
|
Tidy fails. Also, the WPT manifest needs to be updated. Please read |
sendilkumarn
force-pushed
the
image-area
branch
from
1d2569c to
15522a0
Compare
|
@jdm Does the test look good to you? |
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
The test is a good start, but we can write it without timeouts and separate the cases that it is testing.
| return true; | ||
| }; | ||
| document.addEventListener("DOMContentLoaded", function() { | ||
| test.step(function() { |
There was a problem hiding this comment.
document.addEventListener("DOMContentLoaded", test.step_func(function() {
img.click();
});| img.onclick = function(e) { | ||
| img_clicked = true; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
img.onclick = test.step_func_done();|
|
||
| <img usemap="#img_no_map" src="2x2.png" id="img_no"/> | ||
| <script> | ||
| var test = async_test("ImageShouldFindTheMapEvenWhenItIsNotItsSibiling"); |
There was a problem hiding this comment.
async_test("Image should find a non-sibling map")
| </map> | ||
| <img usemap="#img_map" src="2x2.png" id="img"/> | ||
|
|
||
| <img usemap="#img_no_map" src="2x2.png" id="img_no"/> |
There was a problem hiding this comment.
img and area elements are self-closing; we don't need the / at all.
| }); | ||
| test.step_timeout(function() { | ||
| assert_true(img_clicked, "Image click expected"); test.done(); | ||
| }, 500); |
There was a problem hiding this comment.
We can remove this with the earlier changes.
|
|
||
| var img_no = document.getElementById('img_no'); | ||
| var img_no_map_clicked = false; | ||
| img_no.onclick = function(e) { |
There was a problem hiding this comment.
img_no.onclick = test2.unreached_func("click event should not be fired for image without map");
| assert_true(img_clicked, "Image click expected"); test.done(); | ||
| }, 500); | ||
|
|
||
| var img_no = document.getElementById('img_no'); |
There was a problem hiding this comment.
We should declare a separate async_test for the case with no map.
| }); | ||
| test.step_timeout(function() { | ||
| assert_false(img_no_map_clicked, "Image no map click expected"); test.done(); | ||
| }, 500); |
There was a problem hiding this comment.
document.addEventListener("DOMContentLoaded", test2.step(function() {
img_no.click();
});
window.addEventListener("load", test2.step_func_done(function() {
assert_false(img_no_map_clicked, "Image no map click expected");
});
sendilkumarn
force-pushed
the
image-area
branch
4 times, most recently
from
e76ad45 to
d578772
Compare
linting errors replaced with map fixes comments moving to document added test cases linting and updating manifest changing test cases linting fixes manifest update linting fixes splitting the test cases into two
|
Thanks @jdm for the awesome help 👍 |
|
added tests and the builds are green. |
|
@bors-servo: r+ |
|
📌 Commit 31dafcc has been approved by |
highfive
added
S-awaiting-merge
making image element areas good at finding areas <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15884 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15904) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev |
highfive
removed
the
S-awaiting-merge
6 participants
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is