Closed
Bug 1342941
Opened 7 years ago
Closed 7 years ago
Implement the geometry editor in the new box model
Categories
(DevTools :: Inspector, defect, P2)
Tracking
(firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 fixed, firefox55 verified)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | fixed |
firefox55 | --- | verified |
People
(Reporter: mboldan, Assigned: gl)
References
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
zer0
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
[Affected versions]: - Firefox 54.0a1 (2017-02-27) [Affected platforms]: - Windows 10x64, Mac OS X 10.11.6, Ubuntu 16.04 x86 [Steps to reproduce]: 1. Launch firefox. 2. Go to http://bgrins.github.io/devtools-demos/inspector/positions.html. 3. Right click on a text from Absolute positioning area, or from Relative positioning area and select Inspect Element option. 4. Go to Computed section and observe the Edit position button (https://i.imgur.com/zd6pgW5.png). [Expected result]: - The Edit position button is correctly displayed. [Actual result]: - The Edit position button is not available. [Regression range]: This is a regression. Last good revision: fcef571d22c3ad151a62cb1cad6d751f79a72446 First bad revision: fd85a5aafa9fdc2fbe012408e996f573308fb697 Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fcef571d22c3ad151a62cb1cad6d751f79a72446&tochange=fd85a5aafa9fdc2fbe012408e996f573308fb697
Comment 1•7 years ago
|
||
This is one of the known regressions linked to Bug 1336198, thanks for filing!
Blocks: 1336198
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gl
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Summary: The 'Edit position' button from Computed -> Box model is not available → Implement the geometry editor in the new box model
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=445ebc9da1520ae207324c037786d01804bd7238
Assignee | ||
Comment 4•7 years ago
|
||
This was the old code for reference https://hg.mozilla.org/mozilla-central/rev/c856a31d1f71
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. https://reviewboard.mozilla.org/r/118058/#review121026 Looks good to me, just few nits. I'd like also to test locally applying the patch before give the r+, but it seems it needs to be rebased. Gabriel, could you push the rebased patch? Thanks! ::: devtools/client/inspector/boxmodel/box-model.js:227 (Diff revision 1) > + > + if (!enabled) { > + return; > + } > + > + let nodeFront = this.inspector.selection.nodeFront; Nit: you can use destructuring here. ::: devtools/client/inspector/boxmodel/box-model.js:363 (Diff revision 1) > + * Toggles on/off the geometry editor for the current element when the geometry editor > + * toggle button is clicked. > + */ > + onToggleGeometryEditor: Task.async(function* () { > + let { markup, selection, toolbox } = this.inspector; > + let nodeFront = this.inspector.selection.nodeFront; ditto. ::: devtools/client/inspector/boxmodel/box-model.js:367 (Diff revision 1) > + let { markup, selection, toolbox } = this.inspector; > + let nodeFront = this.inspector.selection.nodeFront; > + let state = this.store.getState(); > + let enabled = !state.boxModel.geometryEditorEnabled; > + > + this.highlighters.toggleGeometryHighlighter(nodeFront); I believe you have to `yield` here. ::: devtools/client/inspector/boxmodel/components/BoxModelInfo.js:30 (Diff revision 1) > + onToggleGeometryEditor: PropTypes.func.isRequired, > }, > > mixins: [ addons.PureRenderMixin ], > > + onGeometryEditorToggle(e) { I think this shuffle of terms is confusing, we should stick to a specific naming convention. If you have done such because we want to call the namesake method on `this.props`, then I suggest to write directly: ```js onToggleGeometryEditor() { this.props.onToggleGeometryEditor(); } ``` We have done the same in both memory tool and RDM.
Updated•7 years ago
|
Flags: needinfo?(gl)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gl)
Attachment #8844752 -
Flags: review?(zer0)
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. https://reviewboard.mozilla.org/r/118058/#review121026 > Nit: you can use destructuring here. I chose to just leave it as is since we don't normally destructure nodeFronts elsewhere. Mainly, keeping the change for consistency but it also seems unnecessary to destructure a single variable. > ditto. Ditto > I believe you have to `yield` here. Change the function to not use Task.async. I don't think this is necessary since we don't need to block for any results from the toggleGeometryHighlighter async call.
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. https://reviewboard.mozilla.org/r/118058/#review121170 Looks good to me! Thanks for the rebasing. I was able to test the few edge cases I had in mind. I faced some issues, but I don't think they're related at all to this patch.
Attachment #8844752 -
Flags: review?(zer0) → review+
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. https://reviewboard.mozilla.org/r/118058/#review121026 > I chose to just leave it as is since we don't normally destructure nodeFronts elsewhere. Mainly, keeping the change for consistency but it also seems unnecessary to destructure a single variable. I don't think we should treat nodeFronts differently from other variables unless there is a specific reason for that, in that case it should be stated maybe in a comment. Also, we often destructure a single variable in our codebase (see BoxModelInfo.js in this very patch, for example), and I like we're avoiding repetition to do so. Said that, I don't have strong feelings about it, as soon as we are consistent in the file itself, so feel free to leave as is if you prefer this way. > Ditto as above. :)
Comment 10•7 years ago
|
||
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/55dd9efeeb00 Implement the geometry editor toggle in the new box model. r=zer0
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/55dd9efeeb00
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 12•7 years ago
|
||
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(gl)
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 13•7 years ago
|
||
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Comment 14•7 years ago
|
||
Verified fixed using the latest Nightly 55.0a1 (2017-03-21) on Ubuntu 16.04, Mac OS X 10.12 and Windows 10 x64.
Flags: needinfo?(brindusa.tot)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. Approval Request Comment [Feature/Bug causing the regression]: Bug 1336198 [User impact if declined]: Missing geometry editor feature [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]: No [Is the change risky?]: No. [Why is the change risky/not risky?]: We have this feature previously with unit tests and we re-added the same code to get this working again. [String changes made/needed]: None
Flags: needinfo?(gl)
Attachment #8844752 -
Flags: approval-mozilla-aurora?
Comment 16•7 years ago
|
||
Comment on attachment 8844752 [details] Bug 1342941 - Implement the geometry editor toggle in the new box model. Fix a regression related to geometry editor in devtools and was verified. Aurora54+.
Attachment #8844752 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f72463998068
Flags: in-testsuite+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•