Clicking on the [...] should expand the markup container

RESOLVED FIXED in Firefox 63

Status

P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 63

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 months ago
Created attachment 9004771 [details] [diff] [review]
1486801.patch
Attachment #9004771 - Flags: review?(jdescottes)
Comment on attachment 9004771 [details] [diff] [review]
1486801.patch

Review of attachment 9004771 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
Could we have another version that doesn't rely on _onToggle and covers the feature with a mochitest?
Either expanding one of the browser_markup_toggle_*.js tests or creating a new one.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +78,2 @@
>    this.onDisplayBadgeClick = this.onDisplayBadgeClick.bind(this);
> +  // this.onExpandBadgeClick = this.onExpandBadgeClick.bind(this);

remove this commented out line

@@ +168,5 @@
>      open.appendChild(closingBracket);
>  
> +    this.expandBadge = this.doc.createElement("span");
> +    this.expandBadge.classList.add("markup-expand-badge");
> +    this.expandBadge.addEventListener("click", this.container._onToggle);

I think we should improve this a bit
- expose a toggle() method with no _ on markupContainer
- wrap the call in a method that is bound here (eg onExpandBadgeClick) so that we don't rely on the fact that the other class binds this method by chance

::: devtools/client/themes/markup.css
@@ +444,5 @@
>  .markup-badge[data-display="inline-grid"]:not(.active):hover,
>  .markup-badge[data-event]:focus,
> +.markup-badge[data-event]:hover,
> +.expandable.collapsed .markup-expand-badge:focus::before,
> +.expandable.collapsed .markup-expand-badge:hover::before {

Since we are removing all references to .close::before, can you remove the following selector 
  .theme-selected ~ .editor .close::before
at line 394? (should probably have been done in bug 1471460)
Attachment #9004771 - Flags: review?(jdescottes)
Comment on attachment 9005108 [details] [diff] [review]
1486801.patch [1.0]

Review of attachment 9005108 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for updating the patch!

::: devtools/client/inspector/markup/views/markup-container.js
@@ +60,5 @@
>      this.buildMarkup();
>  
>      this.elt.container = this;
>  
> +    this.expandContainer = this.expandContainer.bind(this);

this bind is not necessary but feel free to leave it.
Attachment #9005108 - Flags: review?(jdescottes) → review+

Comment 5

5 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93112d1b45cb
Clicking on the [...] should expand the markup container. r=jdescottes
(Assignee)

Updated

5 months ago
Flags: needinfo?(gl)

Comment 7

5 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64418bdd4bd2
Clicking on the [...] should expand the markup container. r=jdescottes

Comment 8

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64418bdd4bd2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.