Closed Bug 1135830 Opened 9 years ago Closed 9 years ago

[Customizer] Highlight currently selected elements

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: drs, Assigned: djf)

References

Details

(Whiteboard: [spark])

Attachments

(1 file)

link to patch on github
41 bytes, text/x-github-pull-request
justindarc
: review+
Details | Review 
We should highlight the currently selected element, in content, like the Fx dev tools do.
Priority: -- → P1
Blocks: spark-customizer
No longer blocks: 1133835
Assignee: administration → dflanagan
Justin has already written highlighting code, but it is commented out because it isn't working correctly. With the highlighting call uncommented, what I see is that when I tap on the root <html> node to open it up, the entire window is highlighted as expected, but the <html> node in the tree does not open up to show the body and head elements.

With logging, I see that there are two different click events being delivered and _handleSelected() is being called twice in fxos-customizer.js.  If I set a breakpoint there, then I can see that after the first click event the tree does open up, then it closes again when the second event is sent.  Presumably the gaia-dom-tree component is also getting the two events and is opening and closing itself.

The first click event is synthetic (isTrusted: false). The second one is real. 

If I can figure out where the synthetic click event is coming from, I can probably solve this.
The double event is just because gaia-dom-tree emits a poorly-named 'click' event. We should just change this to 'select' or something. It causes the highlighting code to be called twice, but does not cause the open/close that I was seeing.

I now suspect that the dom tree itself is detecting the tree change that occurs when the highlight is shown and is resetting itself.
(In addition to renaming the event, gaia-dom-tree should probably also call stopPropagation on the click and contextmenu events that it handles)
Doug has moved the mutation observer out out of the gaia-dom-tree element and into the customizer app, and it is no longer triggering rendering when the highlight is shown.

So just uncommenting the highlighting code, makes it work in a basic way.

I've got to fix z-index issues so that the highlight covers the app but not the customizer.

And I've got to deal with scrolling so that the highlight moves with the element it is above.  Or maybe I can do that by changing the way the highlight is rendered so that it is position: absolute instead of position:fixed.

Also, I should probably try to scroll to make the selected element visible at the top of the screen
Justin,

This patch makes the highlighter work.

Once Doug's mutation observer changed, I just had to uncomment the the highlighting code to get something basic working.

Then I had to fix z-index and scrolling issues, which was what took the time.  In the process I refactored the highlighter component to simplify it.

I don't know why github won't create a PR that just has my one commit in it. There were merge conflicts when I did a 'git pull upstream master' and I had to manually resolve those. I have no idea why that happened, but I suppose that has something to do with the fact that I get 4 commits in this PR when I only pushed one commit to my branch.  I don't know what is going on. Feel free to cherry-pick just the one commit and land it yourself it that would be cleaner.
Attachment #8571686 - Flags: review?(jdarcangelo)
Comment on attachment 8571686 [details] [review]
link to patch on github

LGTM
Attachment #8571686 - Flags: review?(jdarcangelo) → review+
Landed the patch to the customizer repo: https://github.com/fxos/customizer/commit/a6ce1d3ebdae54b3869be5c472f33f16b97ac2ba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 1139916
Depends on: 1139920
Whiteboard: [lightsaber] → [ignite]
Whiteboard: [ignite] → [spark]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: