Closed Bug 1329068 Opened 3 years ago Closed 3 years ago

Fix layout issue for PropertiesView

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 verified)

VERIFIED FIXED
Firefox 53
Iteration:
53.5 - Jan 23
Tracking Status
firefox53 --- verified

People

(Reporter: ntim, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(5 files)

No description provided.
Assignee: nobody → ntim.bugs
ntim, 

I'd like to change this bug since it impacts on many network details panels which are using PropertiesView (headers, cookies, params, response, security panels). IMO, this issue belongs to PropertiesView itself and some improvements we can do for it.

* Support scrollable feature for PropertiesView and source editor.
* PropertiesView is able to fill the available vertical space.
* If source editor is displayed, it should fill the remaining vertical space within PropertiesView.
Component: Developer Tools: Shared Components → Developer Tools: Netmonitor
Summary: Make TreeTable component use flexbox → Fix layout issue for PropertiesView
Whiteboard: [netmonitor][triage]
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]
Patch for fixing layout issue is submitted. I know there are some tricks and somehow dirty as well to introduce a display: flex int body and force tr and td to be display: block, but it works great!

One tiny issue I saw that scrollbar always show up even if child element is not such tall. For example if there is a source editor display in response panel, scrollbar will be displayed as attachment. In fact, the table is also scrollable if you move your mouse on "Response payload" section.
Is there any potential risk if we use these tricks to workaround the layout issues for scrollable table?

ntim, would you mind if I steal this bug? I'm expecting that you could give me some suggestions because you're the css expert who might qualify for reviewing this challenging layout issue :)
This patch also fix:

* Display colon ":" correctly after tree label like VariablesView
* Colon ":" is no longer to append after tree section
* Tree cell layout looks more like VariablesView, but I don't know whether it's what we want (see attachment).
Priority: P1 → P2
QA Contact: ciprian.georgiu
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105672

Thanks for working on this Ricky!

See my inline comments.

Honza

::: devtools/client/netmonitor/shared/components/headers-panel.js:205
(Diff revision 2)
>          )
>        );
>      }
>  
>      return (
> -      div({},
> +      div({ className: "panel-container" },

Shouldn't the new 'panel-container' be applied to all side panels?

::: devtools/client/themes/netmonitor.css:1081
(Diff revision 2)
>      font-size: 85%;
>    }
>  }
>  
> -/* Overwrite tree-view cell colon and use l10n string instead */
> -.treeTable .treeLabelCell::after {
> +/* Overwrite tree-view cell colon */
> +#security-tabpanel .treeTable .treeLabelCell::after,

Why the new CSS selector? The previous one didn't have the right priority? An explanation would woule help here.

::: devtools/client/themes/netmonitor.css:1133
(Diff revision 2)
>  
>  /* Make treeTable fill parent element and scrollable */
>  .tree-container .treeTable {
>    position: absolute;
>    display: block;
>    overflow-y: auto;

Shouldn't overflow-y prop be removed here? See the attached screenshot showing double-scrollbar in the Response view (might happen in Params view as well I guess).
Attachment #8826903 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Created attachment 8827163 [details]
> double-scrollbar-win10.png

Yes, that is the same issue which I has mentioned on comment 4. We should remove scrollbar if there is only a source editor but I have no idea why scrollbar still appears.

Do you know any possible reason causes that happen?
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105466

I haven't tested the patch yet, I'll clear review? once I do.

Here are a few comments in the meantime.

::: devtools/client/themes/netmonitor.css:1081
(Diff revision 2)
> -.treeTable .treeLabelCell::after {
> +#security-tabpanel .treeTable .treeLabelCell::after,
> +.treeTable .tree-section .treeLabelCell::after {

I should be able to understand these selectors without testing the patch, so can this comment be clarified? or maybe the selectors could be improved ?

::: devtools/client/themes/netmonitor.css:1109
(Diff revision 2)
> -   * Give a fixed panel container height in order to force tree view scrollable */
> -  height: calc(100vh - 48px);
>    display: flex;
>    flex-direction: column;
> +  width: 100%;
> +  flex: 1 1 auto;

flex-grow: 1 should be enough ?

::: devtools/client/themes/netmonitor.css:1140
(Diff revision 2)
>    right: 0;
>    bottom: 0;
>    left: 0;
>  }
>  
> +.tree-container .treeTable tbody {

I would prefer to switch to divs, and apply the table styles on them by default.

Then we could override the styles for properties-view.

Also, it would be nice if the properties-view CSS could be moved out to its own file so we can re-use it.
I'd be fine to do this as a follow up though.

::: devtools/client/themes/netmonitor.css:1150
(Diff revision 2)
> +
> +.tree-container .treeTable tr {
> +  display: block;
> +}
> +
> +.tree-container .treeTable td:last-child {

Same here, this selector should be understandable without testing the patch.

Can it be more explicit or at least have a comment to explain it?

::: devtools/client/themes/netmonitor.css:1155
(Diff revision 2)
> +.tree-container .treeTable td:last-child {
> +  width: 100%;
> +}
> +
> +.tree-container .treeTable .editor-row-container,
> +.tree-container .treeTable tr:last-child td[colspan="2"] {

Same here.
Attachment #8826903 - Flags: review?(ntim.bugs)
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105676

::: devtools/client/themes/netmonitor.css:1130
(Diff revision 2)
>  .tree-container .treeTable {
>    position: absolute;
>    display: block;
>    overflow-y: auto;
>    top: 0;
>    right: 0;
>    bottom: 0;
>    left: 0;

Hopefully this can be removed when the netmonitor is turned into an HTML file

::: devtools/client/themes/netmonitor.css:1298
(Diff revision 2)
>    -moz-box-flex: 1;
> +  -moz-box-orient: vertical;

I wonder if these 2 properties still apply. This element is a CSS flexbox, and the 2 properties are XUL flexbox.
Assignee: ntim.bugs → rchien
Status: NEW → ASSIGNED
Iteration: --- → 53.5 - Jan 23
Priority: P2 → P1
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105466

> I would prefer to switch to divs, and apply the table styles on them by default.
> 
> Then we could override the styles for properties-view.
> 
> Also, it would be nice if the properties-view CSS could be moved out to its own file so we can re-use it.
> I'd be fine to do this as a follow up though.

IIUC, you prefer to switch the implementation of TreeView from table to divs. If that's right, it would impact on other components which has used TreeView (JSON viewer, Webconsole and DOM panel).
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105676

> I wonder if these 2 properties still apply. This element is a CSS flexbox, and the 2 properties are XUL flexbox.

There is a comment to describe that is a workaround which is supposed to be removed on we migrate to HTML. This XUL flexbox is used for fixing layout issue between XUL and HTML since we append a HTML element under XUL. You can try to remove this and see what happens.
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105672

> Shouldn't the new 'panel-container' be applied to all side panels?

Right now, panel-container is used for fixing layout issue if the panel layout is summary (only headers panel) + search box + properties view. So it's necessary for headers, cookies, params and response panels.
Honza, do you have any opinion in comment 12?

I think it might have to put a lot of effort to deal with layout issue for TreeView. I'd prefer to stick with this workaround which just overwrites TreeView's table with flex rather than refactors TreeView implementation. However, it's a great idea to use divs instead of table. I'd be find to report a follow-up for refactoring TreeView.

If I misunderstood, please correct me.
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Shouldn't overflow-y prop be removed here? See the attached screenshot
> showing double-scrollbar in the Response view (might happen in Params view
> as well I guess).

overflow-y is necessary here in order to support scrollable TreeView.

STR for testing scrollbar TreeView:

1. Visit https://rickychien.github.io/testing-site/
2. Open devtools -> network -> params panel
3. Click ?view=json request in request list
4. Shrink the height of devtools panel as short as possible so make it unable to take up all JSON properties
5. Then you should see a scrollbar

So, we still need to find a better solution for removing this redundant scrollbar which has been described on comment 4.
(In reply to Ricky Chien [:rickychien] from comment #15)
> Honza, do you have any opinion in comment 12?
> 
> I think it might have to put a lot of effort to deal with layout issue for
> TreeView. I'd prefer to stick with this workaround which just overwrites
> TreeView's table with flex rather than refactors TreeView implementation.
> However, it's a great idea to use divs instead of table. I'd be find to
> report a follow-up for refactoring TreeView.

We've been chatting with :ntim on IRC and I am supporting the idea. The first step should be supporting table layout using CSS styles applied on <div>s. This way we'd support backward compatibility since the result would be exactly the same as using table elements (<table>, <tr>, etc.). JSON Viewer, DOM panel, etc. should work nicely after this step.
In the second step, we can experiment with flex layout and introduce a new mode where the three view uses it. We might use this e.g. on Network side panels as an experiment/application. In the future, if the flex box works nicely for all our cases, we can remove support for tables and only use flex box.

I don't know how much work the first step represents and I think that we should finish this bug first. Make sure that the Network side-panels work as expected and move towards our main goal, i.g. refactoring the Net monitor. After that, ntim can start experimenting with the TreeView.

Thoughts?

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> (In reply to Ricky Chien [:rickychien] from comment #15)
> > Honza, do you have any opinion in comment 12?
> > 
> > I think it might have to put a lot of effort to deal with layout issue for
> > TreeView. I'd prefer to stick with this workaround which just overwrites
> > TreeView's table with flex rather than refactors TreeView implementation.
> > However, it's a great idea to use divs instead of table. I'd be find to
> > report a follow-up for refactoring TreeView.
> 
> We've been chatting with :ntim on IRC and I am supporting the idea. The
> first step should be supporting table layout using CSS styles applied on
> <div>s. This way we'd support backward compatibility since the result would
> be exactly the same as using table elements (<table>, <tr>, etc.). JSON
> Viewer, DOM panel, etc. should work nicely after this step.
> In the second step, we can experiment with flex layout and introduce a new
> mode where the three view uses it. We might use this e.g. on Network side
> panels as an experiment/application. In the future, if the flex box works
> nicely for all our cases, we can remove support for tables and only use flex
> box.
> 
> I don't know how much work the first step represents and I think that we
> should finish this bug first. Make sure that the Network side-panels work as
> expected and move towards our main goal, i.g. refactoring the Net monitor.
> After that, ntim can start experimenting with the TreeView.
> 
> Thoughts?

Agree. I'm fine with leaving this workaround first and report two separate bugs for improving TreeView itself. First one is aiming at replacing table with divs and second one is aiming at using flex to layout the scrollable TreeView.

Let's go back to review this bug and I need your help to come out with a better solution for the problem on comment 17.
I found that the scrollbar appears because tbody is taller than table. A workaround we can fix the issue that is to set tbody's height to calc(100% - 3px). I guess the probably root cause is due to anonymous table around tbody when we change display: table into flex or block.

http://stackoverflow.com/questions/30851336/setting-overflow-scroll-on-a-table-with-display-flex/30851678#30851678

The anonymous table is unable to be selectable, so right now I apply this workaround for getting rid of redundant scrollbar.
Attached image Issues
I can't seem to scroll with this patch, overflow-y: auto on treeTable fixes the issue (I don't know how it affects Windows though, Honza might know).

Also, the text isn't clipped with an ellipsis. text-overflow: ellipsis; + overflow:hidden; won't be enough in this case because the span seems as wide as the text is.

I think the proper way to solve the issues would really be switching to divs and using flexbox layout at all levels. I also really think this bug would be more actionable with the netmonitor switched to HTML.
(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> I don't know how much work the first step represents and I think that we
> should finish this bug first. Make sure that the Network side-panels work as
> expected and move towards our main goal, i.g. refactoring the Net monitor.
> After that, ntim can start experimenting with the TreeView.
It's not much work really, I have a patch that does that. It probably needs some test fixing, but that's probably all.
(In reply to Ricky Chien [:rickychien] from comment #19)
> Let's go back to review this bug and I need your help to come out with a
> better solution for the problem on comment 17.

Works well for me on Win10.

Doesn't work properly on OSX. The extent of the scrollbar is smaller so, I can't scroll to the very bottom. E.g. in case of the Params panel and the test-case [1] I can't see the last `null:null` row. The smaller the height of the panel is the more wrong is the extent (the more rows at the bottom I can't see).

[1] https://rickychien.github.io/testing-site/

(In reply to Tim Nguyen :ntim from comment #23)
> It's not much work really, I have a patch that does that. It probably needs
> some test fixing, but that's probably all.

As we quickly discussed on IRC, let's ship a workaround so, we are not blocking Network panel refactoring and fix CSS properly after HTML conversion. I also want to see proper fix for this!

Honza
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105932

Works for me now (tested on Win10 and OSX).

R+ assuming this is a workaround and we'll find better solution as soon as the Network panel is fully converted to HTML.

Thanks for working on this Ricky!
Honza
Attachment #8826903 - Flags: review?(odvarko) → review+
(In reply to Tim Nguyen :ntim from comment #22)
> Also, the text isn't clipped with an ellipsis. text-overflow: ellipsis; +
> overflow:hidden; won't be enough in this case because the span seems as wide
> as the text is.

Yes, I remember I mentioned this on https://bugzilla.mozilla.org/show_bug.cgi?id=1317650#c46. We cannot fully support XUL's adjustable crop feature (set crop=“center” attribute) without JavaScript. So we used Reps's built-in crop-limit props to set a fixed length ellipsis string.

> I think the proper way to solve the issues would really be switching to divs
> and using flexbox layout at all levels. I also really think this bug would
> be more actionable with the netmonitor switched to HTML.

Agree, we can file a separate bug under TreeView component to handle this issue:)
Comment on attachment 8826903 [details]
Bug 1329068 - Fix layout issue for PropertiesView

https://reviewboard.mozilla.org/r/104732/#review105982

r+ with cropping issue fixed.
Attachment #8826903 - Flags: review?(ntim.bugs) → review+
Thanks for reviewing! I updated my patch with browser_net_post-data-01.js failure fixed. It will set a 400px height for devtools panel in order to make CodeMirror display full text.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b1352c021f8
Fix layout issue for PropertiesView r=Honza,ntim
https://hg.mozilla.org/mozilla-central/rev/0b1352c021f8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I can no longer see issues with PropertiesView on latest Aurora 53.0a2 (2017-02-02) under Windows 10 x64, Ubuntu 16.04 LTS x64 and Mac OS X 10.11.6. Closing this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.