Closed
Bug 1353319
Opened 8 years ago
Closed 7 years ago
Render the HTML preview within the Response side-panel
Categories
(DevTools :: Netmonitor, enhancement)
DevTools
Netmonitor
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: Honza, Assigned: brandon.cheng)
References
Details
(Keywords: dev-doc-complete, good-first-bug)
Attachments
(5 files)
Follow up for bug 1350229
HTML preview was previously implemented as a side panel and removed in bug 1350229.
We should re-introduce this feature and render the preview within the existing Response side panel.
Honza
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Whiteboard: [devtools-html] [triage] → [netmonitor] [triage]
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Updated•8 years ago
|
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Comment 1•7 years ago
|
||
Still valid according to the last comment in bug 1247392
Keywords: good-first-bug
Assignee | ||
Comment 2•7 years ago
|
||
Hi. May I be assigned this bug?
Updated•7 years ago
|
Assignee: nobody → brandon.cheng
Comment 3•7 years ago
|
||
Hey Brandon, have you made progress on this bug ?
Let us know if you need any help.
Flags: needinfo?(brandon.cheng)
Assignee | ||
Comment 4•7 years ago
|
||
Hi Tim!
Thanks for checking in. I have an initial version that simply restores the previous functionality in the Response panel, but it replaces the "Response payload" component that's currently there. I saw that the latest Nightly changed the behavior of JSON previews to show both the "PropertiesView" component and the raw "Response payload". (It previously showed just the PropertiesView.)
http://imgur.com/a/0DF13
I'd like for HTML previews to behave the same and show both the preview and the raw response payload, so I'm working on that at the moment. I'm currently trying to understand more of the netmonitor codebase so my patch doesn't disrupt or change too much, but I do have a question that would help me do this faster.
Assuming that mimicing the JSON tree view behavior would be desired, would it make sense to extend PropertiesView to show an arbitrary component (the iframe in this case), or use something other than the PropertiesView when showing an HTML preview?
Thank you!
Flags: needinfo?(brandon.cheng)
Comment 5•7 years ago
|
||
Hi Brandon, thanks for the update!
(In reply to Brandon Cheng from comment #4)
> I'd like for HTML previews to behave the same and show both the preview and
> the raw response payload
Yes, this is the expected behaviour.
> Assuming that mimicing the JSON tree view behavior would be desired, would
> it make sense to extend PropertiesView to show an arbitrary component (the
> iframe in this case), or use something other than the PropertiesView when
> showing an HTML preview?
You could rename renderRowWithEditor [0] to renderRowWithExtras then make it support iframe previews, or more generic components if that's not too complicated. The function shows how we add support for rendering Editors in the property tree.
[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/properties-view.js#86
With Firefox 55 hiting release, many more users are being affected by this. This is a essential functionality for front-end and PHP developers. Some even had to move an entire development team to chrome because of this! If this could get some priority, please prioritize it!
See: https://www.reddit.com/r/firefox/comments/6h7n50/why_was_http_response_preview_removed_from/
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Danilo from comment #8)
> This is a essential functionality for front-end and PHP developers.
I fully agree. I'll do my best to get the initial patch in by tomorrow morning. Let me note quickly the patch will have to through review and won't hit the stable release until Firefox 57.
Comment 10•7 years ago
|
||
In case the review is positive and this lands the next days, maybe it could be requested to uplift the patch to 56?
Sebastian
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 11•7 years ago
|
||
Here's an update. With Tim's help I was able to get the HTML preview to show alongside the "Response payload" panel. I'm now tackling an issue with the .editor-row-container being larger than it's supposed to be. It has a CSS height of 100%, causing the container (or response panel) to overflow. This isn't too noticable on the current latest Nightly, but you can see how the horizontal scroll bar is hidden in an unscrollable area when panning left and right. The response payload label takes up the space that the horizontal scrollbar would normally be. Adding the HTML preview exacerbated this in that now parts of the raw payload are unreachable.
I thought about filing this as a separate bug, but I'll try to fix it here first. I can definitely file a bug with a more detailed report and a video if someone would like though.
I think a vertical flexbox container would solve this well. Going to try and tackle that today; I'll submit the patch as soon as that works.
Assignee | ||
Comment 12•7 years ago
|
||
Haha, problem solved! Turns out the container was already a vertical flexbox.
Comment 13•7 years ago
|
||
Thanks for the hard work Brandon! Feel free to upload the patch and ask for feedback on this CSS issue. It sounds like you might want a overflow: hidden or auto depending on the use case. Using a vertical flexbox or flexbox in general is usually a good idea.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #13)
> Thanks for the hard work Brandon! Feel free to upload the patch and ask for
> feedback on this CSS issue.
Thank to you guys for putting letting us put this feature back in! Commits should be uploaded now.
I have to say, ReviewBoard is a breath of fresh air from the old patch system.
Comment 17•7 years ago
|
||
Thanks again for the patch. We will try to get this reviewed asap.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8903233 [details]
Bug 1353319 - Make SourceEditor flex when next to an HTML preview.
https://reviewboard.mozilla.org/r/175026/#review180232
Thanks for working on this! The UI looks fine to me.
::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1041
(Diff revision 1)
> +
> +.html-preview {
> + height: 100%;
> +}
> +
> +.html-preview iframe {
Can you add a white background to the iframe ? That would be a sensible default for an iframe IMO, especially for the dark theme.
::: devtools/client/netmonitor/src/components/properties-view.js:115
(Diff revision 1)
> + if (level >= 1 && (path.includes(EDITOR_CONFIG_ID) ||
> + path.includes(HTML_PREVIEW_ID))) {
nit: I feel like this would be easier to read:
if ((path.includes(EDITOR_CONFIG_ID) || path.includes(HTML_PREVIEW_ID))
&& level >= 1) {
::: devtools/client/netmonitor/src/components/response-panel.js:25
(Diff revision 1)
> const JSON_SCOPE_NAME = L10N.getStr("jsonScopeName");
> const JSON_FILTER_TEXT = L10N.getStr("jsonFilterText");
> const RESPONSE_IMG_NAME = L10N.getStr("netmonitor.response.name");
> const RESPONSE_IMG_DIMENSIONS = L10N.getStr("netmonitor.response.dimensions");
> const RESPONSE_IMG_MIMETYPE = L10N.getStr("netmonitor.response.mime");
> +const RESPONSE_PREVIEW = L10N.getStr("responsePreview");
nit: move RESPONSE_PREVIEW after RESPONSE_PAYLOAD (alphabetical ordering)
::: devtools/client/netmonitor/src/components/response-panel.js:113
(Diff revision 1)
> + // Detecting HTML isn't as simple as detecting JSON since problematic
> + // HTML may still render fine. We'll do our best and go off of the MIME type.
> + isHTML(mimeType) {
> + return /text\/html/.test(mimeType);
> + },
I wonder why this is needed, since the original implementation of the HTML Preview panel uses:
```
const { Filters } = require("../utils/filter-predicates");
...
Filters.html(this.props.request);
```
Attachment #8903233 -
Flags: review?(ntim.bugs)
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8903232 [details]
Bug 1353319 - Render HTML preview within Response side-panel.
https://reviewboard.mozilla.org/r/175024/#review180248
::: commit-message-fb224:1
(Diff revision 1)
> +Bug 1353319 - Fix SourceEditor component overflow; r?ntim
Seems like your change breaks the ability to see the raw JSON response for long JSON files:
Example: http://api.duckduckgo.com/?q=valley+forge+national+park&format=json&pretty=1
Attachment #8903232 -
Flags: review?(ntim.bugs)
Comment 20•7 years ago
|
||
Here's the JSON issue I'm talking about, the "Response Payload" section isn't expandable.
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #19)
> Comment on attachment 8903232 [details]
> Bug 1353319 - Fix SourceEditor component overflow;
>
> https://reviewboard.mozilla.org/r/175024/#review180248
>
> ::: commit-message-fb224:1
> (Diff revision 1)
> > +Bug 1353319 - Fix SourceEditor component overflow; r?ntim
>
> Seems like your change breaks the ability to see the raw JSON response for
> long JSON files:
>
> Example:
> http://api.duckduckgo.com/?q=valley+forge+national+park&format=json&pretty=1
That's quite bad. Apologies, I got too excited at fixing the bottom horizontal scroll bar and didn't test well enough.
So I see three ways that the SourceEditor component can behave while in the Response panel.
1. Have a container with "height: 100%;", and scrolling within the SourceEditor layer. This is what we do now, but is problematic since you have two scroll contexes and it's hard to reach the end of the SourceEditor. At least on macOS, the container's scrollbar and the SourceEditor's scrollbar are hidden by default, so you have to awkwardly scroll on the labels to get around in some cases. I'll attach a video showing this.
2. Have a container with "flex-grow: 1;" and share space with another flex item. This is what I thought would work well, but doesn't work for the long JSON case you showed me. It's obvious to me now why this didn't work, since the remaining flexible area to grow is 0 in this case.
3. Have a container with "height: auto;" and only 1 scroll context on the container. I would like to be given the okay to explore this since it would work well for SourceEditor's current usage and here. I also think 1 scroll context is better usability wise.
To do #3 though, we have to remove the mouse wheel listener on the Editor component. It looks like we're currently capturing it in order to update the line numbers gutter position.
https://dxr.mozilla.org/mozilla-central/source/devtools/client/sourceeditor/editor.js#338
Assignee | ||
Comment 22•7 years ago
|
||
Here's a video of the problem with #1.
https://cloudup.com/cKaJ0c3IKYE
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Updated•7 years ago
|
Comment 23•7 years ago
|
||
Honza, could you take a look at this ?
Flags: needinfo?(ntim.bugs) → needinfo?(odvarko)
Comment 24•7 years ago
|
||
This seems close to be landable, it would be great to try and have it for Firefox 57!
Brandon: maybe we could simply update your current patch and set `min-height: 50%;` to the selector `.tree-container .treeTable tr:last-child.editor-row-container`?
This way the editor will still be visible for long JSON responses.
We can create a separate bug to fix the remaining issues you raised.
Flags: needinfo?(brandon.cheng)
Reporter | ||
Comment 25•7 years ago
|
||
(In reply to Brandon Cheng from comment #21)
> 3. Have a container with "height: auto;" and only 1 scroll context on the
> container. I would like to be given the okay to explore this since it would
> work well for SourceEditor's current usage and here. I also think 1 scroll
> context is better usability wise.
Yes, agree this sounds promising.
However, from longer-term perspective, I'd vote for using an accordion
component e.g. share the one used by Debugger tool. We might also learn from
how Firebug did this. Some analysis needed.
In any case, I tried suggestion from Julian (comment #24) and it looks like
good enough workaround for now. If you do it, I can R+ both patches and
we can land yet in this cycle. The rest of the work can be done in a follow up.
Thanks for working on this!
Honza
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #25)
> In any case, I tried suggestion from Julian (comment #24) and it looks like
> good enough workaround for now. If you do it, I can R+ both patches and
> we can land yet in this cycle. The rest of the work can be done in a follow
> up.
>
> Thanks for working on this!
> Honza
Thanks for the response Honza. Julian, I appreciate your help as well.
I just pushed something else I've been working on the last few days. I think it's a bit better UI-wise than setting "min-height: 50%;", but not as clean since it relies on a conditional CSS class.
If this alternative isn't ideal, I have no qualms with removing that workaround going Julian's solution.
Flags: needinfo?(brandon.cheng)
Reporter | ||
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8903232 [details]
Bug 1353319 - Render HTML preview within Response side-panel.
https://reviewboard.mozilla.org/r/175024/#review184918
Thanks for working on this!
I am seeing some double-scrollbar issues, but I don't think that there is much we can do about that given the architecture of the PropertiesView. I think that the situation can become a lot easier if we adopt Acordion for all Net side panels.
R+ assuming try is green
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f028b37c33fb9003787e331926f35fef5ec0c8
Please, file a follow up bug related to the double scrollbar issues. It might be fixed by using the Acordion, but it shouldn't be forgotten what these issues are.
Honza
Attachment #8903232 -
Flags: review?(odvarko) → review+
Reporter | ||
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8903233 [details]
Bug 1353319 - Make SourceEditor flex when next to an HTML preview.
https://reviewboard.mozilla.org/r/175026/#review184922
Attachment #8903233 -
Flags: review?(odvarko) → review+
Reporter | ||
Comment 31•7 years ago
|
||
Here is another case where I am seeing double scroll bar (Win10)
It's there if the response payload is bigger than the available area. The second scrollbar is there because the Response Payload section header is not included in 100% height and causes overflow.
Should be mentioned in the follow up report.
Honza
Reporter | ||
Comment 32•7 years ago
|
||
I am seeing this test failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f028b37c33fb9003787e331926f35fef5ec0c8&selectedJob=130990653
251 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_cyrillic-02.js | The text shown in the source editor is correct. -
Honza
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> I am seeing this test failure:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=88f028b37c33fb9003787e331926f35fef5ec0c8&selectedJob=1
> 30990653
>
> 251 INFO TEST-UNEXPECTED-FAIL |
> devtools/client/netmonitor/test/browser_net_cyrillic-02.js | The text shown
> in the source editor is correct. -
>
>
> Honza
I've spent a few days tackling this test and can't seem to get it passing, so I think I have to ask for technical help on this one.
The test can't find the cyrillic text since the HTML preview pushes the source code preview down. And the SourceEditor uses a virtual DOM to only load what's visible. I'm trying to scroll down the source editor a few lines to load more lines into the real DOM, but can't seem to get it working with WheelEvent.
const event = {
deltaMode: WheelEvent.DOM_DELTA_PAGE,
deltaY: 1.0,
lineOrPageDeltaY: 1
}
yield EventUtils.synthesizeWheel(
document.querySelector(".CodeMirror-scroll"),
10,
10,
event
);
```
Scrolling down without using EventUtils seems to work and get the test working.
document.querySelector(".CodeMirror-scroll").scrollBy(0, 200);
But I imagine that we should be using the mock events instead while testing. I'm still toying around with a few things to try and get the EventUtil event working, but I'm not sure why it's not scrolling this specific component yet.
Flags: needinfo?(odvarko)
Reporter | ||
Comment 35•7 years ago
|
||
@Ricky: any tips what could be wrong here?
Honza
Flags: needinfo?(odvarko) → needinfo?(rchien)
Comment 36•7 years ago
|
||
I this test is not asserting the HTML preview, some suggestions:
- collapse the preview pane
- or increase the devtools toolbox height
Comment 37•7 years ago
|
||
(In reply to Brandon Cheng from comment #34)
> (In reply to Jan Honza Odvarko [:Honza] from comment #32)
> > I am seeing this test failure:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=88f028b37c33fb9003787e331926f35fef5ec0c8&selectedJob=1
> > 30990653
> >
> > 251 INFO TEST-UNEXPECTED-FAIL |
> > devtools/client/netmonitor/test/browser_net_cyrillic-02.js | The text shown
> > in the source editor is correct. -
> >
> >
> > Honza
>
> I've spent a few days tackling this test and can't seem to get it passing,
> so I think I have to ask for technical help on this one.
>
> The test can't find the cyrillic text since the HTML preview pushes the
> source code preview down. And the SourceEditor uses a virtual DOM to only
> load what's visible. I'm trying to scroll down the source editor a few lines
> to load more lines into the real DOM, but can't seem to get it working with
> WheelEvent.
>
> const event = {
> deltaMode: WheelEvent.DOM_DELTA_PAGE,
> deltaY: 1.0,
> lineOrPageDeltaY: 1
> }
> yield EventUtils.synthesizeWheel(
> document.querySelector(".CodeMirror-scroll"),
> 10,
> 10,
> event
> );
> ```
What you can try is synthesizing the mousewheel event, then using waitForDOM to wait for the editor to load.
You might also want to double check if the editor is properly expanded (the element that's expanded in the test might change with the new HTML preview).
See this test: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_jsonp.js#71
Flags: needinfo?(brandon.cheng)
Comment 38•7 years ago
|
||
Alternatively, you can just use `node.scrollTop` to scroll to the destination you want, and then remember to use waitForDOM for waiting the text.
waitUntil() [1] is another good method of polling a given condition through gecko thread mechanism. I'm not sure whether the position of cyrillic text is predictable. If it's not, waitUntil() might be your best friend to help you keep scrolling the editor content down and polling the text until it appears.
[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_image-tooltip.js#85
Flags: needinfo?(rchien)
Comment 40•7 years ago
|
||
Any update on this issue?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903233 [details]
Bug 1353319 - Make SourceEditor flex when next to an HTML preview.
https://reviewboard.mozilla.org/r/175026/#review180232
Thank you for reviewing this and your help! I'll push the changes as soon as my build finishes and I test that they work.
> Can you add a white background to the iframe ? That would be a sensible default for an iframe IMO, especially for the dark theme.
Makes sense.
> nit: I feel like this would be easier to read:
>
> if ((path.includes(EDITOR_CONFIG_ID) || path.includes(HTML_PREVIEW_ID))
> && level >= 1) {
I agree, thanks!
> I wonder why this is needed, since the original implementation of the HTML Preview panel uses:
>
> ```
> const { Filters } = require("../utils/filter-predicates");
> ...
> Filters.html(this.props.request);
> ```
Ah thank you. It looks like I didn't look at the patch removing the original implementation carefully enough.
Assignee | ||
Comment 45•7 years ago
|
||
I think my response to Tim's initial review didn't go through until I hit publish on ReviewBoard just now. So the above comment was from a while ago.
I just pushed a new commit that grabs CodeMirror's content through a method the library provides. It fixes the test by comparing against buffered content in memory rather than content rendered to the DOM. The commit description contains more details about other things I tried, like EventUtils.sendWheelAndPaint and scrollBy.
I also amended the first two commits so my patches are based off of the latest nightly.
(In reply to Ricky Chien [:rickychien] from comment #38)
> Alternatively, you can just use `node.scrollTop` to scroll to the
> destination you want, and then remember to use waitForDOM for waiting the
> text.
>
> waitUntil() [1] is another good method of polling a given condition through
> gecko thread mechanism. I'm not sure whether the position of cyrillic text
> is predictable. If it's not, waitUntil() might be your best friend to help
> you keep scrolling the editor content down and polling the text until it
> appears.
This was a good suggestion, but I don't there's an easy way to tell when the cyrillic text is rendered. It's predictably about 200px down, but I'm hesitant to hard-code that into the test.
(In reply to Tim Nguyen :ntim from comment #37)
> You might also want to double check if the editor is properly expanded (the
> element that's expanded in the test might change with the new HTML preview).
>
> See this test:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> test/browser_net_jsonp.js#71
I added a sleep in the middle of the test to observe what was happening. The editor was properly expanded, just shorter than it was before I added the preview. Waiting for ".CodeMirror-code" didn't work since CodeMirror seems to only add lines when they become visible after scroll. I arrived at this conclusion by getting the value of document.querySelector(".CodeMirror-code").textContent in the browser toolbox minutes after loading the editor. This makes sense to me as libraries like react-virtualized use this as a performance optimization.
Flags: needinfo?(brandon.cheng)
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8903232 [details]
Bug 1353319 - Render HTML preview within Response side-panel.
https://reviewboard.mozilla.org/r/175024/#review199262
::: devtools/client/netmonitor/src/components/PropertiesView.js:127
(Diff revision 3)
> + )
> + );
> + }
> +
> + // Skip for editor config and HTML previews
> + if (path.includes(EDITOR_CONFIG_ID) || path.includes(HTML_PREVIEW_ID)
nit: add brackets () around `path.includes(EDITOR_CONFIG_ID) || path.includes(HTML_PREVIEW_ID)` to avoid ambiguity
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 50•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #29)
> Please, file a follow up bug related to the double scrollbar issues. It
> might be fixed by using the Acordion, but it shouldn't be forgotten what
> these issues are.
>
> Honza
Filed as bug 1412607.
Reporter | ||
Comment 51•7 years ago
|
||
mozreview-review |
Comment on attachment 8923134 [details]
Bug 1353319 - Fix cyrillic text test after re-adding the HTML preview.
https://reviewboard.mozilla.org/r/194324/#review200434
Looks good to me!
R+
Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc599cadf216781f5093fc9f7eb7dea39a827d0&selectedJob=141091153
There is one failure, but I don't know how it could be related.
Honza
Attachment #8923134 -
Flags: review?(odvarko) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 55•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #51)
> There is one failure, but I don't know how it could be related.
That test seems to be passing when I run it locally. On a separate note, I just rebased, resolved some merge conflicts, and pushed to mozreview. Seems to still pass on my local machine after rebasing.
Is there anything additional I should do to get this merged into mozilla-central? Or should we investigate that failed debugger test further?
Updated•7 years ago
|
Keywords: checkin-needed
Comment 56•7 years ago
|
||
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bab59d6b58f
Render HTML preview within Response side-panel. r=Honza
https://hg.mozilla.org/integration/autoland/rev/edbc03f93f6a
Make SourceEditor flex when next to an HTML preview. r=Honza
https://hg.mozilla.org/integration/autoland/rev/64bd4d75004d
Fix cyrillic text test after re-adding the HTML preview. r=Honza
Keywords: checkin-needed
Comment 57•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4bab59d6b58f
https://hg.mozilla.org/mozilla-central/rev/edbc03f93f6a
https://hg.mozilla.org/mozilla-central/rev/64bd4d75004d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 58•7 years ago
|
||
Hi,
This bug is marked as solved but I cannot see any difference. Is this anything to do to have the render ?
Thank you,
Comment 59•7 years ago
|
||
This is not solved. I get this with v.57 and had it with 56 as well.
I even tried with an all new profile to see if it would solve anything. It did not.
/E
Comment 60•7 years ago
|
||
(In reply to Emil from comment #59)
>
> This is not solved. I get this with v.57 and had it with 56 as well.
>
> I even tried with an all new profile to see if it would solve anything. It
> did not.
>
> /E
RESOLVED FIXED in Firefox 59
^^^^^^^
As it says on the top, we have to wait till v59
Comment 61•7 years ago
|
||
OK, for the answer. I will wait for a while. I just installed 57. Weight and sea says the whale...
Comment 62•7 years ago
|
||
Is possible to uplift this to 58, as suggested in comment #10?
Comment 63•7 years ago
|
||
ni regarding the uplift. Developers are really missing this feature.
Sebastian
Flags: needinfo?(brandon.cheng)
Comment 64•7 years ago
|
||
Thinking about it, it's probably better to ask Honza about the uplift.
Sebastian
Flags: needinfo?(brandon.cheng) → needinfo?(odvarko)
Reporter | ||
Comment 66•7 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #64)
> Thinking about it, it's probably better to ask Honza about the uplift.
I understand, but the patch is rather bigger and I'd recommend to
wait for 59
Honza
Flags: needinfo?(odvarko)
Comment 67•7 years ago
|
||
I have reproduced this bug with Nightly 55.0a1 (2017-04-04) on Windows 8.1 , 64 Bit !
This bug's fix is Verified with latest Nightly !
Build ID 20171127100433
User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0
[bugday-20171122]
Comment 68•7 years ago
|
||
Hi, is it fixed? For me, I am using laravel and when I dd (die dump), it still return HTML even though I force it into 500 state
Comment 69•7 years ago
|
||
(In reply to yuko.pangestu from comment #68)
> Hi, is it fixed? For me, I am using laravel and when I dd (die dump), it
> still return HTML even though I force it into 500 state
Not sure you're talking about the same thing. This issue asks for a preview for HTML contents within the Response tab when a network request is selected, which got implemented for Firefox 59 (currently on Nightly channel). This is independent of the returned HTTP status code.
Sebastian
Comment 70•7 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #69)
> (In reply to yuko.pangestu from comment #68)
> > Hi, is it fixed? For me, I am using laravel and when I dd (die dump), it
> > still return HTML even though I force it into 500 state
>
> Not sure you're talking about the same thing. This issue asks for a preview
> for HTML contents within the Response tab when a network request is
> selected, which got implemented for Firefox 59 (currently on Nightly
> channel). This is independent of the returned HTTP status code.
>
> Sebastian
Hi Sebastian, yeah basically I am talking about HTML render, but I don't know that Mozzila has nightly, which I currently download right now, However I have 3 firefox on my computer, regular, quantum and nightly, should I keep them both? Or for development purpose I can uninstall the quantum?
Thanks
Comment 71•7 years ago
|
||
Can this be patched in before Firefox 59? This is a pretty bad bug for web devs.
Comment 72•7 years ago
|
||
(In reply to directrix1@gmail.com from comment #71)
> Can this be patched in before Firefox 59? This is a pretty bad bug for web
> devs.
I already asked. See the answer in comment 66.
Sebastian
Comment 73•7 years ago
|
||
For anyone else asking about patching this in before v59, know that you can get Firefox v59 *now* at https://www.mozilla.org/en-US/firefox/channel/desktop/
Developer mode is Release + 1, while Nightly is Release + 2. As of now we are on Firefox v57 still, so you'll want to grab Nightly. Or if you can wait, v59 will hit Release channel in mid-March of 2018.
Comment 74•7 years ago
|
||
I've documented this:
Added a section to the Network Monitor page to show how this feature works:
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#HTML_preview
Added a note to the FX59 rel notes
https://developer.mozilla.org/en-US/Firefox/Releases/59#Changes_for_web_developers
Let me know if this is OK. Thanks!
One more question — I also noticed that the Stack trace tab wasn't documented; what version of Fx was that added in?
Flags: needinfo?(odvarko)
Keywords: dev-doc-needed → dev-doc-complete
Reporter | ||
Comment 75•7 years ago
|
||
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #74)
> I've documented this:
>
> Added a section to the Network Monitor page to show how this feature works:
> https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#HTML_preview
>
> Added a note to the FX59 rel notes
> https://developer.mozilla.org/en-US/Firefox/Releases/
> 59#Changes_for_web_developers
>
> Let me know if this is OK. Thanks!
Looks great, thanks!
> One more question — I also noticed that the Stack trace tab wasn't
> documented; what version of Fx was that added in?
Bug 1350234, Firefox 55
Honza
Flags: needinfo?(odvarko)
Comment 76•7 years ago
|
||
I'd like to say thank you for re-adding this feature, its very helpful. However, i'd like to ask that the requirement for the Content-type header to be html before the preview will be displayed be removed.
The use case being that if i'm developing an API that returns a JSON response, and theres a error in my code, the framework will generate a response in html but the content-type header for json will remain, and thus the html preview will not appear and then i cannot view easily the error generated by the framework.
Thank you.
Comment 77•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #75)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #74)
>
> > One more question — I also noticed that the Stack trace tab wasn't
> > documented; what version of Fx was that added in?
> Bug 1350234, Firefox 55
Cool, thanks. I've made sure that tab is documented too on the page, with the correct version of Fx cited for when it was added.
Reporter | ||
Comment 78•7 years ago
|
||
(In reply to Tam Denholm from comment #76)
> I'd like to say thank you for re-adding this feature, its very helpful.
> However, i'd like to ask that the requirement for the Content-type header to
> be html before the preview will be displayed be removed.
>
> The use case being that if i'm developing an API that returns a JSON
> response, and theres a error in my code, the framework will generate a
> response in html but the content-type header for json will remain, and thus
> the html preview will not appear and then i cannot view easily the error
> generated by the framework.
Please, file a new bug. This one is already closed, thanks!
Honza
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•