Closed Bug 1541278 Opened 7 months ago Closed 3 months ago

Markup View's background flash effect: use yellow from Box Model shift-hover highlight

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: victoria, Assigned: vikas_mahato, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(7 files)

The orange currently used as Markup's dom-change highlight doesn't match the rest of the DevTools theme. It would be great if we could apply the new yellow highlights that were created for the Box Model<->Rules highlight.

In both places, I'd also like to see a longer animation time so that it's a bit more clear what's happening. Could we try twice as long?

Thanks Victoria. This seems like a good first bug that Micah could mentor.
Please go through our contribution docs first and then feel free to ask any questions you might have: http://docs.firefox-dev.tools/

Mentor: mtigley
Type: defect → enhancement
Keywords: good-first-bug
Priority: -- → P3

Hi,
Can I work on this bug?

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #2)

Hi,
Can I work on this bug?

Thank you for interest in this bug. I'll assign it to you now.

Assignee: nobody → vikasmahato0
Status: NEW → ASSIGNED

We need to change the colour of the orange highlights to yellow. Is this right?

Can you point out the relevant files where I should start looking?

Flags: needinfo?(mtigley)

Hi Vikas, to see how the background color is added to the flashing element in the markup view you can go here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/views/markup-container.js#657 .

The flashElementOn and flashElementOff functions take an optional background class argument to modify the background color we are flashing. You can see the function definitions here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/utils.js#20 and see how they work.

The box-model highlight property uses the yellow highlight color that's needed. You can see how this is done here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1580

Hope this helps!

Flags: needinfo?(mtigley)
Attached image BothOrangeAndYellow.png

Hi,
I did the changes described in the patch but I am getting both orange and yellow highlights. Are the flashElementOn and flashElementOff called from some other places as well?

Also I was wondering how to attach a breakpoint inside those methods in the browser toolbox. Any documentation regarding debugging Devtools javascript would be useful.

Flags: needinfo?(mtigley)

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #7)

Are the flashElementOn and flashElementOff called from some other places as well?

It looks like they are also being called here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/views/element-editor.js#195

Also I was wondering how to attach a breakpoint inside those methods in the browser toolbox. Any documentation regarding debugging Devtools javascript would be useful.

The starting point for the DevTools debugger can be found at: https://developer.mozilla.org/en-US/docs/Tools/Debugger

To debug the JavaScript code for DevTools though, you'll need to select the Web Developer submenu in the Firefox "Tools" Menu, from there, select the "Browser Toolbox" menu item and you should see another Developer Tools window popup. The shortcut to open this toolbox for a local build of Firefox on MacOS is: shift+option+CMD+I Once this is done, navigate to the Debugger tab and you can start setting breakpoints in the methods you'd like to observe.

Flags: needinfo?(mtigley)

Thanks,
That helped a lot. I have uploaded a patch for review. Its at https://phabricator.services.mozilla.com/D32864

Attached image Low-vision-text.png

Related to feedback on your patch, Vikas.

Hi,
When we select a an element that is about to be hidden, the div is flashing but the text remains white. Other scenarios seem to be working fine except for this one. Any idea why this might be happening?

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #12)

Created attachment 9069184 [details]
Screen Recording 2019-06-01 at 10.42.02 PM.mov

Hi,
When we select a an element that is about to be hidden, the div is flashing but the text remains white. Other scenarios seem to be working fine except for this one. Any idea why this might be happening?

Good catch! Thank you for uploading a video demonstrating this.

I'll investigate why this is happening. I imagine the color value is being overridden by another rule somewhere.

Hi,
I have updated the patch with the requested changes.

Attached image black_text.png

Hi Victoria, we ran into an accessibility issue regarding the text being barely visible when a selected node is flashed for light theme. You can see this in the screenshot at Comment 11 . To get around this, we decided to apply black for the text color when a selected element or its attribute is being flashed.

Let us know what you think about it!

Flags: needinfo?(victoria)

Oh, nice catch! (One thing I'm wondering is if white-on-dark-yellow would make more sense here - but I'm guessing the translucent yellow being used in the dark mode version of flashes won't be bright enough. Assuming that's the case, I think we should ship this as is and I'll worry about further polish later.)

Flags: needinfo?(victoria)
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52bdbeb64586
Markup View's background flash effect: use yellow from Box Model shift-hover highlight. r=mtigley

Hi,
I see that the changeset has been backed out due to failure of a mochitest. Is there anything I can do to fix that?

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #20)

Hi,
I see that the changeset has been backed out due to failure of a mochitest. Is there anything I can do to fix that?

Hi Vikas, I made a few suggestions on how to fix this in your revision. Please let me know if you have any questions!

Flags: needinfo?(mtigley)

Hi,
Thanks for your comments. I have updated the patch accordingly.

(In reply to Vikas Prasad Mahato [:vikas_mahato] from comment #22)

Hi,
Thanks for your comments. I have updated the patch accordingly.

Thank you again on your patience with this bug. I approved the patch, but just to be sure I'm going to push this to Try.

Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ad1135a1609
Markup View's background flash effect: use yellow from Box Model shift-hover highlight. r=mtigley
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Regressions: 1567335
You need to log in before you can comment on or make changes to this bug.