Closed Bug 1278774 Opened 4 years ago Closed 4 years ago

Breadcrumbs overlaps on the element hiding helper button

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox47 unaffected, firefox48 unaffected, firefox49 affected, firefox50 affected)

RESOLVED INVALID
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- affected
firefox50 --- affected

People

(Reporter: magicp.jp, Assigned: gasolin)

References

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160607071209

Steps to reproduce:

1. Start Nightly with the following add-ons
 - Adblock Plus "https://addons.mozilla.org/firefox/addon/adblock-plus"
 - Element hiding helper for Adblock Plus "https://addons.mozilla.org/firefox/addon/elemhidehelper"
2. Open devtools > Inspector


Actual results:

Breadcrumbs overlaps on the element hiding helper button. In 48.0b1, It does not overlap on that. 


Expected results:

Don't overlap.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [devtools-html] [triage]
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
This extension adds a (XUL) toolbarbutton inside #inspector-breadcrumbs-toolbar before the #inspector-breadcrumbs element.

However, the #inspector-breadcrumbs element is now position: absolute to work around a XUL/html interaction, thus the overlap. (See Bug 1259812 Comment 18)
Priority: P1 → P2
Assignee: nobody → gasolin
Attached image partial fix
WIP: With following change the icon could show without overlapping, 
Though the left padding(20px) of `.breadcrumbs-widget-item` shows a large gap. 

> --- a/devtools/client/themes/inspector.css
> +++ b/devtools/client/themes/inspector.css
> @@ -37,7 +37,8 @@
>    padding: 0px;
>    border-bottom-width: 0px;
>    border-top-width: 1px;
> -  display: block;
> +  display: flex;
> +  flex-direction: row;
>    position: relative;
>  }
> 
> @@ -52,7 +53,7 @@
>    /* Break out of the XUL flexbox, so the splitter can still shrink the
>       markup view even if the contents of the breadcrumbs are wider than
>       the new width. */
> -  position: absolute;
> +  position: relative;
>    top: 0;
>    left: 0;
Status: NEW → ASSIGNED
adblock plus add the toolbox-button xul before the element with `breadcrumbs-widget-container` class.

Besides the change mentioned in comment 3, I compensate the first breadcrumbs-widget-item's left padding when breadcrumbs-widget-container is not the first element

https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/widgets.css#246
Comment on attachment 8769540 [details]
Bug 1278774 - Breadcrumbs overlaps abp helper button;

https://reviewboard.mozilla.org/r/63392/#review60386

::: devtools/client/themes/inspector.css:40
(Diff revision 1)
>  
>  #inspector-breadcrumbs-toolbar {
>    padding: 0px;
>    border-bottom-width: 0px;
>    border-top-width: 1px;
> -  display: block;
> +  display: flex;

I don't know enough about the interaction between CSS flex and XUL flex to say if doing 

<parent style='display: flex'><child style='position: relative'>

sufficiently 'breaks out' in the same way that this does:

<parent style='display: block'><child style='position: absolute'>

But it seems to not change things for me locally so should be OK, I think.  Going to forward this on for Patrick to take a look to make sure he's aware of this potential change

::: devtools/client/themes/widgets.css:127
(Diff revision 1)
>  }
>  
> +/* Bug 1278774 - compenssate first breadcrumbs-widget-item's left padding when
> +   breadcrumbs-widget-container is not the first element */
> +:not(.breadcrumbs-widget-container) + .breadcrumbs-widget-container {
> +  margin-inline-start: -20px;

Not sure I understand why this needed.  I think it's OK if there's a space between the ABP button and the first breadcrumb (if that's all this is preventing I don't think we need to special case it for the addon).
Attachment #8769540 - Flags: review?(bgrinstead)
Attachment #8769540 - Flags: feedback?(pbrosset)
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
> Not sure I understand why this needed.  I think it's OK if there's a space
> between the ABP button and the first breadcrumb (if that's all this is
> preventing I don't think we need to special case it for the addon).

Yes if a space between the ABP button and the first breadcrumb is OK, I can remove that style.
Comment on attachment 8769540 [details]
Bug 1278774 - Breadcrumbs overlaps abp helper button;

https://reviewboard.mozilla.org/r/63392/#review61888

I'm not comfortable changing the positioning scheme of the breadcrumbs because we are (at least for now) in a mixed XUL+HTML world where layout is weird.
The absolute positioning of the breadcrumbs inside its toolbar is nice because it works fine and allows us to break out of the XUL flex layout mechanism.

As noted below in my comments, this change breaks one of the key features of the breadcrumbs widget: scrolling.

Instead of finding a way to accomodate the ABP button automatically in our CSS, I think it would be much easier for the addon to override one CSS property:

```
#inspector-breadcrumbs {
  left: 30px;
}
```

30px or whatever width is the button.
Only pushing the left edge of the breadcrumbs works fine and allows to preserve our absolute positioning.

So, we should either reach out to the authors and see if this is a change they can make, or we can have this as a rule in our CSS in case the #inspector-breadcrumbs isn't first child in #inspector-breadcrumbs-toolbar, but if we do this, then we'd have to choose and hard code a width.

::: devtools/client/themes/inspector.css:41
(Diff revision 1)
>  #inspector-breadcrumbs-toolbar {
>    padding: 0px;
>    border-bottom-width: 0px;
>    border-top-width: 1px;
> -  display: block;
> +  display: flex;
> +  flex-direction: row;

`row` is the default `flex-direction` anyway, so no need to define it here.

::: devtools/client/themes/inspector.css:56
(Diff revision 1)
> -  position: absolute;
> +  position: relative;
>    top: 0;
>    left: 0;
>    bottom: 0;
>    right: 0;

This change breaks the breadcrumbs autoscrolling. Now the `#inspector-breadcrumbs` element does not fit into the available space, and instead seems to expand to all the width it requires, and therefore there are no arrows to scroll through the breadcrumbs anymore.
So we need to find another solution.

::: devtools/client/themes/widgets.css:126
(Diff revision 1)
> +:not(.breadcrumbs-widget-container) + .breadcrumbs-widget-container {
> +  margin-inline-start: -20px;
> +}

Agreed with Brian, don't think this is necessary.
Attachment #8769540 - Flags: review-
Thanks for feedback!

According to comment 8, the proper solution might be add `#inspector-breadcrumbs` style in the addon (Element hiding helper for Adblock Plus) directly.

```
#inspector-breadcrumbs {
  left: 30px;
}
```

Honza, could you help redirect the request to Wladimir Palant(ABP author)?
Flags: needinfo?(odvarko)
Wladimir, can you please help out with this bug?
See comment #9

Honza
Flags: needinfo?(odvarko) → needinfo?(trev.moz)
(In reply to Fred Lin [:gasolin] from comment #9)
> According to comment 8, the proper solution might be add
> `#inspector-breadcrumbs` style in the addon (Element hiding helper for
> Adblock Plus) directly.
> 
> ```
> #inspector-breadcrumbs {
>   left: 30px;
> }
> ```

That's something I would consider a very bad idea. It's bad enough that Element Hiding Helper has to make assumptions about the Dev Tools layout - this is what backfired here. Making assumptions about styling is worse, this is bound to break things even with some themes, and future Firefox versions are guaranteed to mess everything up.

Either way, looking at the new layout it makes much more sense to have our button after the inspector-element-add-button element if it exists (meaning Firefox 48 and higher). Having the button in the breadcrumbs toolbar was never the intention. I filed https://issues.adblockplus.org/ticket/4255 on this, will put a patch under review there soon.
Flags: needinfo?(trev.moz)
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Status: ASSIGNED → NEW
Our fix has landed. As far as I am concerned this can be resolved as INVALID.
Attachment #8769540 - Flags: feedback?(pbrosset)
Thanks Wladimir!
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
No longer blocks: devtools-html-2
Iteration: 50.4 - Aug 1 → ---
Priority: P1 → --
Whiteboard: [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.