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)

54 Branch
defect

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)

[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
This is one of the known regressions linked to Bug 1336198, thanks for filing!
Blocks: 1336198
Priority: -- → P2
Assignee: nobody → gl
Status: NEW → ASSIGNED
Summary: The 'Edit position' button from Computed -> Box model is not available → Implement the geometry editor in the new box model
This was the old code for reference https://hg.mozilla.org/mozilla-central/rev/c856a31d1f71
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.
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Attachment #8844752 - Flags: review?(zer0)
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 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 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. :)
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
https://hg.mozilla.org/mozilla-central/rev/55dd9efeeb00
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(gl)
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)
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)
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 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: