Closed
Bug 1423098
Opened 7 years ago
Closed 7 years ago
Remove support for SMIL's accessKey feature
Categories
(Core :: SVG, enhancement)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: birtles, Assigned: birtles)
References
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
jcristau
:
approval-mozilla-beta-
|
Details |
59 bytes,
text/x-review-board-request
|
dholbert
:
review+
|
Details |
No other browser implements it and it's been the source of security-related bugs in the past (e.g. bug 704482).
Assignee | ||
Comment 1•7 years ago
|
||
Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/skw-Yj_Pdjk
Keywords: dev-doc-needed,
site-compat
Comment 2•7 years ago
|
||
--> Assigning to birtles, since I think he's intending to work on this per the wording in his intent-to-unship.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
Yes, thanks. Sorry, I was unwell last week so didn't get to it. I'll try to get to it on the plane today.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;
https://reviewboard.mozilla.org/r/207068/#review212938
Thanks! This looks great.
r- for the time being, only because I think we might want to test one more thing (see below), which might want a quick review pass.
::: dom/smil/nsSMILTimeValueSpecParams.h:60
(Diff revision 1)
> - // The repeat iteration (type=REPEAT) or access key (type=ACCESSKEY) to
> - // respond to.
> - uint32_t mRepeatIterationOrAccessKey;
> + // The repeat iteration to respond to.
> + // Only used for REPEAT types.
> + uint32_t mRepeatIteration;
The phrase "REPEAT types" sounds a bit confusing here, since mType has only one (not plural) repeat-flavored value.
Maybe:
"Only used when mType=REPEAT."
?
::: dom/smil/test/test_smilAccessKey.xhtml:20
(Diff revision 1)
> </svg>
> </div>
> <pre id="test">
> <script class="testbody" type="text/javascript">
> <![CDATA[
> /** Test for SMIL accessKey support **/
s/support/lack-of-support/, perhaps?
::: dom/smil/test/test_smilAccessKey.xhtml:25
(Diff revision 1)
> function main()
> {
> - gSvg.pauseAnimations();
> -
> + testBeginValueIsNotSupported('accessKey(a)');
> + testBeginValueIsNotSupported('accesskey(a)');
It's probably worth testing a list-valued begin value as well -- something like the following:
begin="3s; accessKey(a)"
begin="accessKey(a); 1s"
(Are the 3s and 1s values still expected to work there, or will we reject the whole attribute-parse operation?)
Attachment #8936351 -
Flags: review?(dholbert) → review-
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/smil-accesskey-support-has-been-removed/
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;
https://reviewboard.mozilla.org/r/207068/#review213050
Can you explain the changes in this update? It's not clear to me why this affects the behavior. It looks like the code changes here are just adjusting how we report failure from SetBeginOrEndSpec, but it looks like we still end up reporting "some error code" in exactly the same conditions that we did before, so it's not clear to me why this produces a behavior change. Do we react to different error codes differently, up the stack somewhere?
::: dom/smil/nsSMILTimedElement.cpp:1327
(Diff revision 2)
> - nsresult rv = NS_OK;
> - while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
> + bool hadFailure;
> + while (tokenizer.hasMoreTokens()) {
> nsAutoPtr<nsSMILTimeValueSpec>
> spec(new nsSMILTimeValueSpec(*this, aIsBegin));
> - rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> + nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> if (NS_SUCCEEDED(rv)) {
> timeSpecsList.AppendElement(spec.forget());
> + } else {
> + hadFailure = true;
> }
> }
>
> - if (NS_FAILED(rv)) {
> - ClearSpecs(timeSpecsList, instances, aRemove);
> - }
> -
> + // The return value from this is only used to determine if we should print
> + // a console message or not, so we return failure if we had one or more
> + // failures.
> + return hadFailure ? NS_ERROR_FAILURE : NS_OK;
Two things
(1) I think you need to initialize hadFailure to false (optimistically) on the line where you declare it, right?
(Nothing ever sets it to false right now; and it looks like there are cases where we leave it uninitialized right now.)
(2) I didn't initially get the gist of the comment at the end. I think you should mention "so we don't bother distinguishing between different error-flavored nsresult values" or something along those lines.
::: dom/smil/test/test_smilTiming.xhtml:135
(Diff revision 2)
> - testCases.push(StartTimeTest('3;;', 'none'));
> - testCases.push(StartTimeTest('3;; ', 'none'));
> - testCases.push(StartTimeTest(';3', 'none'));
> - testCases.push(StartTimeTest(' ;3', 'none'));
> + testCases.push(StartTimeTest('3;;', 3));
> + testCases.push(StartTimeTest('3;; ', 3));
> + testCases.push(StartTimeTest(';3', 3));
> + testCases.push(StartTimeTest(' ;3', 3));
Looks like this version is now making us accept empty begin/end list entries now -- is that a correct interpretation of this change?
(Is that behavior interoperable/correct? Might merit a code-comment here to document the expected new behavior. Also: if this is a result of the SetBeginOrEndSpec changes, maybe it'd be worth splitting those code-changes and this test-change into a dedicated patch, since those are non-accessKey-specific and have a non-accessKey-related functionality difference? And then the accessKey change can be 100% focused on accessKey)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 8936351 [details]
> Bug 1423098 - Drop support for SMIL accessKey;
>
> https://reviewboard.mozilla.org/r/207068/#review213050
>
> Can you explain the changes in this update? It's not clear to me why this
> affects the behavior. It looks like the code changes here are just adjusting
> how we report failure from SetBeginOrEndSpec, but it looks like we still end
> up reporting "some error code" in exactly the same conditions that we did
> before, so it's not clear to me why this produces a behavior change. Do we
> react to different error codes differently, up the stack somewhere?
The key difference is that we drop the call to ClearSpecs(). Previously we'd call that and end up with no begin/end time (i.e. unresolved) but after this patch we will keep the specs we succeeded in parsing.
> ::: dom/smil/nsSMILTimedElement.cpp:1327
> (Diff revision 2)
> > - nsresult rv = NS_OK;
> > - while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
> > + bool hadFailure;
> > + while (tokenizer.hasMoreTokens()) {
> > nsAutoPtr<nsSMILTimeValueSpec>
> > spec(new nsSMILTimeValueSpec(*this, aIsBegin));
> > - rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> > + nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> > if (NS_SUCCEEDED(rv)) {
> > timeSpecsList.AppendElement(spec.forget());
> > + } else {
> > + hadFailure = true;
> > }
> > }
> >
> > - if (NS_FAILED(rv)) {
> > - ClearSpecs(timeSpecsList, instances, aRemove);
> > - }
> > -
> > + // The return value from this is only used to determine if we should print
> > + // a console message or not, so we return failure if we had one or more
> > + // failures.
> > + return hadFailure ? NS_ERROR_FAILURE : NS_OK;
>
> Two things
>
> (1) I think you need to initialize hadFailure to false (optimistically) on
> the line where you declare it, right?
Yes, sorry. (I wrote this patch to try and keep me awake in the Stylo meeting, but that too failed.)
> (2) I didn't initially get the gist of the comment at the end. I think you
> should mention "so we don't bother distinguishing between different
> error-flavored nsresult values" or something along those lines.
Will do.
> ::: dom/smil/test/test_smilTiming.xhtml:135
> (Diff revision 2)
> > - testCases.push(StartTimeTest('3;;', 'none'));
> > - testCases.push(StartTimeTest('3;; ', 'none'));
> > - testCases.push(StartTimeTest(';3', 'none'));
> > - testCases.push(StartTimeTest(' ;3', 'none'));
> > + testCases.push(StartTimeTest('3;;', 3));
> > + testCases.push(StartTimeTest('3;; ', 3));
> > + testCases.push(StartTimeTest(';3', 3));
> > + testCases.push(StartTimeTest(' ;3', 3));
>
> Looks like this version is now making us accept empty begin/end list entries
> now -- is that a correct interpretation of this change?
More or less. Basically, an empty begin/end list entry will not cause us to error out and drop the whole list of specs. It won't actually be added as a new spec. I should update the commit message.
> (Is that behavior interoperable/correct? Might merit a code-comment here to
> document the expected new behavior. Also: if this is a result of the
> SetBeginOrEndSpec changes, maybe it'd be worth splitting those code-changes
> and this test-change into a dedicated patch, since those are
> non-accessKey-specific and have a non-accessKey-related functionality
> difference? And then the accessKey change can be 100% focused on accessKey)
Yeah, I'll do that.
Comment 10•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #9)
> The key difference is that we drop the call to ClearSpecs().
Ah! I totally overlooked that removal. Thanks for clarifying!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;
https://reviewboard.mozilla.org/r/207068/#review213434
::: commit-message-a44e0:3
(Diff revision 3)
> +This matches the behavior of Chrome at least, and will reduce compatibility
> +issues when we remove accessKey support in the next patch in this series since
> +specifications such as begin="3s; accessKey(s)" will continue to run the
> +animation.
I think this would be clearer/more-accurate with:
s/since/so that/
(IIUC, this change is needed, in order for the animation to continue to run in that scenario.)
Attachment #8936351 -
Flags: review?(dholbert) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8936845 [details]
Bug 1423098 - Drop support for SMIL accessKey;
https://reviewboard.mozilla.org/r/207546/#review213450
r=me
Attachment #8936845 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8fdaff3f0af
Ignore invalid time value specifications rather than failing; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6f8ed92515d6
Drop support for SMIL accessKey; r=dholbert
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8fdaff3f0af
https://hg.mozilla.org/mozilla-central/rev/6f8ed92515d6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 19•7 years ago
|
||
I think we might want to consider uplift to beta here. What do you think, Brian?
(The patches seem pretty safe / minimally-invasive to me. Minimal webcompat risk, given that we were the only ones shipping this feature.)
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;
Approval Request Comment
[Feature/Bug causing the regression]: Bug 587910
[User impact if declined]: Potential for content to perform keylogging in contexts that are expected to be unscripted.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Both patches in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minimally invasive, simply drops a feature that other browsers never implemented, fails softly when no-longer-supported content is encountered.
[String changes made/needed]: None.
Flags: needinfo?(bbirtles)
Attachment #8936351 -
Flags: approval-mozilla-beta?
Comment 21•7 years ago
|
||
It seems pretty late in beta to make a change affecting webcompat, I'd rather give this a chance to bake more than a couple of weeks in beta, i.e. ride the trains to 59.
status-firefox58:
--- → wontfix
Updated•7 years ago
|
Attachment #8936351 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 22•7 years ago
|
||
We don't really have any documentation about SMIL, and it certainly isn't a priority, so I've just added a note to the Fx59 rel notes for this:
https://developer.mozilla.org/en-US/Firefox/Releases/59#SVG_2
Let me know if that's OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•