Closed Bug 1528774 Opened 10 months ago Closed 5 months ago

Remove unnecessary code in GridItem's onGridCheckboxClick

Categories

(DevTools :: Inspector: Layout, enhancement, P3)

enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: nchevobbe, Assigned: armando.ferreira, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files, 1 obsolete file)

On devtools/client/inspector/grids/components/GridItem.js#73-80, there's this piece of code:

  onGridCheckboxClick(e) {
    // If the click was on the svg icon to select the node in the inspector, bail out.
    const originalTarget = e.nativeEvent && e.nativeEvent.explicitOriginalTarget;
    if (originalTarget && originalTarget.namespaceURI === "http://www.w3.org/2000/svg") {
      // We should be able to cancel the click event propagation after the following reps
      // issue is implemented : https://github.com/devtools-html/reps/issues/95 .
      e.preventDefault();
      return;
    }

The referenced issue (https://github.com/devtools-html/reps/issues/95) is resolved, which means we can get rid of this code, and call stopPropagation in onInspectIconClick

onInspectIconClick: (_, e) => {
  e.stopPropagation();
  this.onGridInspectIconClick(grid.nodeFront);
}
Priority: -- → P3

Hi, I am new to the community. I would like to work on this Issue. I made the suggested changed and was wondering if someone could offer some guidance on how to further test the changes suggested to make sure I removed the correct code and the changes didn't affect any other code.

Updated onGridCheckboxClick:

onGridCheckboxClick(e) {
    const {
      grid,
      onToggleGridHighlighter,
    } = this.props;

    onToggleGridHighlighter(grid.nodeFront);
 }
Attached image image.png

Hello sohail!

To manually test this change, you can open a new tab with this as a URL:

data:text/html,<meta charset=utf8><style>html {display: grid;}</style><html>hello</html>

Then, open the inspector, select the layout pane, expand the "grid" section if it's not expanded.
You should see an checkbox with an html label, and an icon right next to it (see attachment).
Clicking on the icon shouldn't check the checkbox.

Hopefully, this is also covered by automated tests.
You can see if everything is okay by running ./mach test devtools/client/inspector/grids/test/ --headless.

Hope I was able to help you, don't hesitate to ask additional questions here or on Slack 🙂

Assignee: nobody → nchevobbe
Assignee: nchevobbe → sohail.n.hameed
Mentor: gl → nchevobbe
Attachment #9049253 - Attachment description: Summary: Bug 1528774 - Removed unnecessary code in GridItem.js file's onGridCheckboxClick funcation and updated the render funcation so that the onInspectIconClick call tiggers the stopPropagation funcation r=nchevobbe → Summary: Bug 1528774 - Removed unnecessary code in GridItem.js file's onGridCheckboxClick function and updated the render function so that the onInspectIconClick call triggers the stopPropagation function r=nchevobbe

Hi Nicolas, Is this still open? Can I take it?

Flags: needinfo?(nchevobbe)

Hello Armando, thanks for your help.
This bug is still open, yes. I assigned it to you, feel free to ask any question :)

Assignee: sohail.n.hameed → armando.ferreira
Flags: needinfo?(nchevobbe)

Removed unnecessary code in GridItem.js file's onGridCheckboxClick function and updated the render function so that the onInspectIconClick call triggers the stopPropagation function.

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ccc84a42f5e3
Remove unnecessary code in GridItem's onGridCheckboxClick. r=nchevobbe
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Attachment #9049253 - Attachment is obsolete: true
QA Whiteboard: [qa-70b-p2]
You need to log in before you can comment on or make changes to this bug.