Closed Bug 1241126 Opened 9 years ago Closed 9 years ago

Should not create newProperty when clicking to blur current property editor

Categories

(DevTools :: Inspector: Rules, defect)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(
URL
)

Details

Attachments

(2 files, 6 obsolete files)

bug1241126.part1.v4.patch
8.38 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
bug1241126.part2.v3.patch
5.69 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
Follow up to 1198326. STRs : - Open the following data URL : > data:text/html,<body>Test<style>body{padding:10px;margin:0px;} - Open the inspector - Select the Rules view - Click on "10px" -> inplace editor appears - Click to the right of the editor (anywhere in the blank space after the input) Expected : The inplace editor should disappear. Actual : The inplace editor disappears AND a new property editor is created at the end of the current RuleEditor section. If any editor is opened in the ruleview, clicking on blank space should not add a new rule. Any "editor" as in editors for selector, css name, css value.
Bug 1241126 - add test for click behavior of text-property-editor;r=gl This commit fixes an inconsistency in text-property-editor.js We had code ensuring that when clicking outside of the nameSpan but inside of the nameContainer, the click would be forwarded to the nameSpan in order to trigger the creation of the inplace editor. And the same for the valueSpan and its container. Implementation was correct for the valueSpan, but faulty for the nameSpan. This commit fixes that, and adds a test checking the editors are created when clicking on the namespan, valuespan or on the textnodes next to them but still included in their containers. (commit needed for this bug as I will need a proper reference to text-property-editor to write an upcoming test)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8710346 - Flags: review?(gl)
URL: data:text/html,<body>Test<style>body{...
Has STR: --- → yes
Attached patch bug1241126.part2.wip.patch (obsolete) — — Splinter Review
This commit actually fixes the UI/UX issues described in the STRs. You can now close the current editor by simply clicking on the "empty" areas of the rule view. I would like feedback on the implementation. Basically using a combination of mousedown + click to decide if creating a new property is ok. It works, but I guess it is a bit hacky. Other solutions I tried are relying on timers, which is even worse. Let me know what you think!
Attachment #8710388 - Flags: feedback?(pbrosset)
Attachment #8710388 - Flags: feedback?(gl)
Comment on attachment 8710346 [details] [diff] [review] bug1241126.part1.patch (add tests about clicks on text-property-editor) Review of attachment 8710346 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/rules/test/browser_rules_edit-property-click.js @@ +4,5 @@ > + > +"use strict"; > + > +// Tests that the property value and name editors can be triggered when > +// clicking on the property-name, the property-value or the colon, semicolon. Can we rearrange 'value' and 'name' and reword the comment to: // Tests that the property name and value editors can be triggered when // clicking on the property-name, the property-value, the colon or semicolon. @@ +37,5 @@ > + yield focusEditableField(view, propEditor.nameSpan, nameRect.width + 1); > + ok(propEditor.nameSpan.inplaceEditor, "Editor created for property name"); > + yield sendCharsAndWaitForFocus(view, ruleEditor.element, ["VK_ESCAPE"]); > + > + info("Test editor is created when clicking on property value"); I think we should either remove the "Test" in the info comment or add that to every info to be consistent. I am more in favor of adding "Test" since it is describing what is to happen. @@ +40,5 @@ > + > + info("Test editor is created when clicking on property value"); > + yield focusEditableField(view, propEditor.valueSpan); > + ok(propEditor.valueSpan.inplaceEditor, "Editor created for property value"); > + // when cancelling a value edition, the text-property-editor will trigger Capitalize first word for comments ::: devtools/client/inspector/rules/views/text-property-editor.js @@ +183,5 @@ > this.nameContainer.addEventListener("click", (event) => { > // Clicks within the name shouldn't propagate any further. > event.stopPropagation(); > + > + // forward clicks on nameContainer to the editable nameSpan Capitalize the first word to be consistent with the comment styles
Attachment #8710346 - Flags: review?(gl) → review+
Thanks for the review Gabriel! Carry over r+ from your review.
Attachment #8710346 - Attachment is obsolete: true
Attachment #8710932 - Flags: review+
Comment on attachment 8710388 [details] [diff] [review] bug1241126.part2.wip.patch Review of attachment 8710388 [details] [diff] [review]: ----------------------------------------------------------------- Even if a little hacky, I don't find this to disturbing either. We use state flags like this in a variety of places already. In fact, when we do, we tend not to define them in the constructor at all. Especially because the only lines of code that use it are in very close proximity. So you could just not declare it at the top, and only when you first set it to true, have a comment that explains why.
Attachment #8710388 - Flags: feedback?(pbrosset) → feedback+
Bug 1241126 - rule-view skip newProp creation when leaving editor;r=gl Add a flag to check if the ruleview was displaying an editor before creating a newProperty editor. A new property editor is now only displayed if no other editor was previously displayed. Added new mochitest to check this use case.
Attachment #8710388 - Attachment is obsolete: true
Attachment #8710388 - Flags: feedback?(gl)
Attachment #8712065 - Flags: review?(gl)
Depends on: 1214177
Attachment #8712065 - Flags: review?(gl) → review+
Simple rebase as the patch was quite old. Carry over r+
Attachment #8710932 - Attachment is obsolete: true
Attachment #8714249 - Flags: review+
Thanks for the review Gabriel! Rebased, try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f5ecdad63a Carry over r+
Attachment #8712065 - Attachment is obsolete: true
Attachment #8714250 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
(just forgot this depended on Bug 1214177)
Rebased on top of Bug 1214177, carry over r+.
Attachment #8714249 - Attachment is obsolete: true
Attachment #8714357 - Flags: review+
Rebased on top of Bug 1214177, carry over r+.
Attachment #8714250 - Attachment is obsolete: true
Attachment #8714358 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8be5815e1c1e https://hg.mozilla.org/mozilla-central/rev/fd4b62ebe725
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: