Closed
Bug 1069829
Opened 10 years ago
Closed 8 years ago
Properties validity shouldn't be checked on the front-end (aka stop using domUtils.cssPropertyIsValid in the client code)
Categories
(DevTools :: Inspector: Rules, enhancement, P1)
DevTools
Inspector: Rules
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: pbro, Assigned: pbro)
References
(Depends on 2 open bugs)
Details
(Whiteboard: [btpp-2016-Q3] [devtools-html])
Attachments
(1 file)
Being remotely connected to a debuggee means that the toolbox shouldn't be making too much assumptions about what the debuggee is. Especially in light of addons like fxdt-adapters that let you connect to webkit and blink debuggees. One such assumption is in the rule-view, when checking of a given property/value is valid: /** * Validate this property. Does it make sense for this value to be assigned * to this property name? This does not apply the property value * * @param {string} [aValue] * The property value used for validation. * Defaults to the current value for this.prop * * @return {bool} true if the property value is valid, false otherwise. */ isValid: function(aValue) { let name = this.prop.name; let value = typeof aValue == "undefined" ? this.prop.value : aValue; let val = parseSingleValue(value); let style = this.doc.createElementNS(HTML_NS, "div").style; let prefs = Services.prefs; // We toggle output of errors whilst the user is typing a property value. let prefVal = prefs.getBoolPref("layout.css.report_errors"); prefs.setBoolPref("layout.css.report_errors", false); let validValue = false; try { style.setProperty(name, val.value, val.priority); validValue = style.getPropertyValue(name) !== "" || val.value === ""; } finally { prefs.setBoolPref("layout.css.report_errors", prefVal); } return validValue; } This is front-end code, meaning that it will always run in a Gecko browser, which means that properties like "-webkit-transition" will always be invalid. Properties validity should always be checked on the server-side.
Comment 1•9 years ago
|
||
The as-authored series (bug 984880) added a few more instances of this. There are more in the rule view, but also one in the rule rewriting code and also some in output-parser.js.
Assignee | ||
Comment 2•8 years ago
|
||
Filter on CLIMBING SHOES
Priority: -- → P2
Whiteboard: [btpp-2016-Q3]
Assignee | ||
Updated•8 years ago
|
Blocks: top-inspector-bugs
Assignee | ||
Comment 3•8 years ago
|
||
This bug is now about removing usages of domUtils.cssPropertyIsValid in the rule-view. The code in comment 0 does not exist anymore and has been replaced with domUtils.cssPropertyIsValid. Here are all the usages in the tree: https://dxr.mozilla.org/mozilla-central/search?q=domUtils.cssPropertyIsValid&redirect=false&case=false So, basically, all related to the inspector (mostly css rule view). We need to go over each one and decide: - does it need to be moved to the server-side if the returned value we expect from cssPropertyIsValid should depend on the browser currently hosting the debuggee page, then yes, this should move to the server, and therefore become async, which could be complex, but needed, - or can it stay on the client-side one example of this is _findCompatibleUnits here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#592 I don't expect this to behave differently depending on the browser engine running the page, so it is safe to keep this on the client-side, but replace it with a JS implementation (see bug 1263287).
Blocks: devtools-html-3
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Assignee | ||
Updated•8 years ago
|
Summary: [rule view] Properties validity shouldn't be checked on the front-end → Properties validity shouldn't be checked on the front-end (aka stop using domUtils.cssPropertyIsValid in the client code)
Updated•8 years ago
|
Severity: normal → enhancement
Comment 4•8 years ago
|
||
The instance in the rule rewriter can easily be made async. Another implementation option is to have the server send a property database to the client at startup. This would avoid difficulties making anything async.
Updated•8 years ago
|
Priority: P2 → --
Whiteboard: [btpp-2016-Q3] → [btpp-2016-Q3] [devtools-html]
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46745/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46745/
Attachment #8741775 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•8 years ago
|
||
Not sure how much of this I'll be able to do, so I'm assigning this bug to me just yet. But here's a first (very simple) part. For this particular usage of cssPropertyIsValid I believe there's no need to go to the server, so I implemented a simple property validation using a DOM element locally. I did not want to move this to a shared helper function just because I don't think we should reuse this in many places. Most of the times, the right answer is to go to the server.
Comment 7•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey Potentially supported units could change I guess, but as you say in Comment 3 it's probably safe across platforms. Forwarding review to Tom though for a second opinion.
Attachment #8741775 -
Flags: review?(bgrinstead) → review?(ttromey)
Comment 8•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey https://reviewboard.mozilla.org/r/46745/#review43407 Looks good, thanks.
Attachment #8741775 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Pending try push for part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b415f588c933&group_state=expanded
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/99fbe8621fa7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 12•8 years ago
|
||
Sorry, forgot to add the leave-open keyword when I landed part 1. The first patch only removed one instance of cssPropertyIsValid in the code, there are other ones in the rule-view, filterWidget and output-parser. The one in the rule-view is the most important and complex one I think. It is used to display a warning sign next to properties that are not valid. Obviously this varies across browsers and should therefore come from the server. Our actor only returns the full css text for a rule, which means that we must parse them into property:value pairs on the client and check the validity of each pair on the client. I checked the chromedevtools' protocol, and the corresponding method not only returns the css text but also the parsed declarations and information about their validity. I believe we should do the same here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Assignee | ||
Comment 13•8 years ago
|
||
I started working on part 2, making the StyleRuleActor's form contain the parsed declarations.
Assignee: nobody → pbrosset
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46745/diff/1-2/
Attachment #8741775 -
Attachment description: MozReview Request: Bug 1069829 - 1 - Remove a usage of domUtils.cssPropertyIsValid in inplace-editor; r=bgrins → MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey
Assignee | ||
Comment 15•8 years ago
|
||
Oh noes.. mozreview thinks this is a new version of the first patch, so it R+'d it and didn't ask for a new review.
Assignee | ||
Updated•8 years ago
|
Attachment #8741775 -
Flags: review+ → review?(ttromey)
Comment 16•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey https://reviewboard.mozilla.org/r/46745/#review46731 ::: devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js:67 (Diff revision 2) > info("Checking that _Copy() returns the correct clipboard value"); > > let expectedPattern = " margin: 10em;[\\r\\n]+" + > " font-size: 14pt;[\\r\\n]+" + > - " font-family: helvetica,sans-serif;[\\r\\n]+" + > - " color: rgb\\(170, 170, 170\\);[\\r\\n]+" + > + " font-family: helvetica, sans-serif;[\\r\\n]+" + > + " color: #AAA;[\\r\\n]+" + I'm curious why the spelling of the color had to change here. I couldn't readily tell from the patch. Was this an existing authored styles bug? Or is it a regression now? ::: devtools/server/actors/styles.js:1195 (Diff revision 2) > }, > > // True if this rule supports as-authored styles, meaning that the > // rule text can be rewritten using setRuleText. > get canSetRuleText() { > - // Special case about:PreferenceStyleSheet, as it is > + return this.type === ELEMENT_STYLE || I have some vague worry about this. I thought there was a problem that could arise if as-authored-style editing was done on element styles. But, I can't remember what that was and I think perhaps always getting the authored text from the element may address whatever it was. Maybe something like disabling and then re-enabling a property won't work? Not sure. ::: devtools/server/actors/styles.js:1473 (Diff revision 2) > > - this.authoredText = newText; > - yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES); > + yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES); > + } > + > + this.authoredText = newText; Is it necessary to set the authored text here for an element style? If so, I think a comment explaining why would be nice. If not, I think it's better to just leave this out, since it's a bit confusing -- because when computing the form we always re-fetch the style from the element anyway (which is the right thing to do).
Attachment #8741775 -
Flags: review?(ttromey) → review+
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Assignee | ||
Comment 17•8 years ago
|
||
Thanks Tom for the review. Sorry for the delay here, but making element style rule as authored made some tests fail, and I've been spending some time fixing those. I have something that works locally. I'm going to ask for another review in a bit because these were not only obvious mistakes.
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46745/diff/2-3/
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey Tom, could you give this another look? You will be mostly interested in this interdiff: https://reviewboard.mozilla.org/r/46745/diff/2-3/ - I had to change layout.js because when you edit margin, border or padding in this view, the element style is changed. Before my patch, this was using the RuleModificationList helper because element style was not as authored. Now it use the RuleRewriter, and that caused some tests to fail and I fixed all the issues. Mostly, I had the problem of the RuleRewriter only supporting one change. So what I did was just sequence them in layout.js. - browser_rules_add-property_02.js was failing intermittently (only when run in a test suite), so I cleaned it up a little bit (it was defining unused styles), and added some safe measures to avoid rejected promises. I'm still unsure if this is due to my changes though. - browser_styleinspector_context-menu-copy-color_01.js this test uses both the rule-view and computed-view. Because the rule-view now uses authored styles for the element style rule, I just used the rgb format to make sure the test expected the same string in both views. - test_styles-modify.html was failing also because we are now using the RuleRewriter for element styles. I fixed the test but made sure it tested the same thing as before. - styles.js: I am not the author of the changes made to this file in this interdiff. Turns out they disabled the auto-hiding of rebase changes in mozreview recently, so I guess these changes are shown here because of that.
Attachment #8741775 -
Flags: review+ → review?(ttromey)
Comment 20•8 years ago
|
||
Comment on attachment 8741775 [details] MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey https://reviewboard.mozilla.org/r/46745/#review48167 Looks good to me. Thank you. ::: devtools/server/actors/styles.js:978 (Diff revisions 2 - 3) > + * > + * @param {Document} document > + * The document in which the style element should be appended. > * @returns DOMElement of the style tag > */ > - get styleElement() { > + getStyleElement: function(document) { I think eslint wants a space before the "(".
Attachment #8741775 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c96822cd293f&group_state=expanded
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb721f4d509f https://hg.mozilla.org/mozilla-central/rev/3b16eb206789
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Iteration: --- → 49.1 - May 9
Priority: P2 → P1
Assignee | ||
Updated•8 years ago
|
No longer blocks: top-inspector-bugs
Depends on: 1270186
Depends on: 1273624
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•