New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements HTMLDialogElement#close #12847
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon. |
Heads up! This PR modifies the following files:
|
@clstl Please revert all the indentation changes. |
@@ -8,5 +8,8 @@ interface HTMLDialogElement : HTMLElement { | |||
attribute DOMString returnValue; | |||
//void show(optional (MouseEvent or Element) anchor); | |||
//void showModal(optional (MouseEvent or Element) anchor); | |||
//void close(optional DOMString returnValue); | |||
|
|||
// https://html.spec.whatwg.org/multipage/#the-dialog-element:dom-dialog-close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link isn't necessary.
It looks like not all the steps in the algorithm is implemented. Is this going to be addressed? Can you also add step annotations above every line of code that it is trying to follow? |
@KiChjang Yes, the only thing left is removal from the 'pending dialog stack'. However, neither 'show' or 'showModal' are implemented. Moreover, I could not seem to find anything related to the way this stack works in the project. Any help would be appreciated. I will annotated the code in the next fixup shortly. |
354484a
to
f152e3f
Compare
r? @nox |
-S-awaiting-review +S-needs-code-changes +S-awaiting-answer Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r8, 1 of 1 files at r9. a discussion (no related file): components/script/dom/htmldialogelement.rs, line 65 [r9] (raw file):
That link should be https://html.spec.whatwg.org/multipage/#dom-dialog-close. components/script/dom/htmldialogelement.rs, line 73 [r9] (raw file):
Why? The spec just stays to abort these steps. components/script/dom/htmldialogelement.rs, line 77 [r9] (raw file):
This should use if element.remove_attribute(&ns!(), &atom!("open")).is_none() {
return;
} components/script/dom/htmldialogelement.rs, line 87 [r9] (raw file):
Use components/script/dom/htmldialogelement.rs, line 88 [r9] (raw file):
Nit: indentation is wrong here. components/script/dom/webidls/HTMLDialogElement.webidl, line 12 [r9] (raw file):
This method doesn't throw. Comments from Reviewable |
The intended scope is to implement close s much as possible. If step 4 cannot be done at this point, then we should at least test for the firing of the close event (step 5). |
@bors-servo try |
Implements HTMLDialogElement#close <!-- Please describe your changes on the following line: --> Implements HTMLDialogElement#close according to [link](https://html.spec.whatwg.org/multipage/#the-dialog-element:dom-dialog-close) --- <!-- 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 #12352 - [X] There are tests for these change <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/12847) <!-- Reviewable:end -->
@nox on it.
test-tidy tells the link might change and showed me this instead |
Didn't tidy complain about the filename in the URL, rather than the URL itself? |
@clstl You probably misread the tidy error, given the other links are like the one I stated:
|
💔 Test failed - linux-rel |
@nox: so It is not just me
This is Step 1 from the test. Should I just check for no returnValue then? |
f152e3f
to
7e18571
Compare
89375aa
to
0539022
Compare
@nox good to go |
@bors-servo r+ |
📌 Commit 0539022 has been approved by |
Implements HTMLDialogElement#close <!-- Please describe your changes on the following line: --> Implements HTMLDialogElement#close according to [link](https://html.spec.whatwg.org/multipage/#the-dialog-element:dom-dialog-close) --- <!-- 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 #12352 - [X] There are tests for these change <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/12847) <!-- Reviewable:end -->
💔 Test failed - linux-rel |
You need to update the test expectations. To do that easily, run the following commands:
|
0539022
to
aa5edd1
Compare
@bors-servo r+ |
📌 Commit aa5edd1 has been approved by |
Implements HTMLDialogElement#close <!-- Please describe your changes on the following line: --> Implements HTMLDialogElement#close according to [link](https://html.spec.whatwg.org/multipage/#the-dialog-element:dom-dialog-close) --- <!-- 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 #12352 - [X] There are tests for these change <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/12847) <!-- Reviewable:end -->
☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev |
Implements HTMLDialogElement#close according to link
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is