Closed Bug 1188667 Opened 5 years ago Closed 5 years ago
[meta] View Source for 2
"I want to be able to view source of apps"
No description provided.
Comment on attachment 8648214 [details] [review] [gaia] DouglasSherk:1188667-view-source > mozilla-b2g:master How do you test this?
I mean, automation tests, of course.
That's a good question. I'm not sure how we can test add-ons. Fabrice, any suggestions?
Flags: needinfo?(drs) → needinfo?(fabrice)
Are the dependent bugs on here hard requirements for 2.5? (because they have the 2.5+ feature flag marked and they haven't been resolved)
We talked a bit on irc. On gecko we have mochitests to check that we inject scripts properly, but it's a bit unclear how we can test that in gaia integration tests. Maybe something like: frameA.html (the 'driver') includes frameB.html, we trigger view-source on frameB.html, and frameA reaches into frameB to check that the content of the view-source iframe is as expected.
A Gij test accompany with the patch would be good.
feature-b2g: --- → 2.5+
Component: Gaia → Gaia::System
Comment on attachment 8653191 [details] [review] [gaia] DouglasSherk:1188667-view-source > mozilla-b2g:master We're no longer using this, instead opting to build the feature into the core OS.
Attachment #8653191 - Attachment is obsolete: true
FYI, No-Jun might be the tester going forward for this feature.
Attachment #8653761 - Attachment description: View Source Window Screenshot → View Source window screenshot
Attachment #8653764 - Attachment description: View source window screenshot 3 → View Source window screenshot 3
Please see the other attachments in this bug for screenshots. Note that we don't have much control over the text styling, colors, etc. as they are all shared with Firefox and come from the 'view-source:' URL scheme. I can, however, change the look of the window, transparency, method to open/close it, etc.
Comment on attachment 8653766 [details] View Source demo video Hey Doug, overall it looks good, just a couple things: - I'm worried how you close the screen isn't obvious enough right now, so instead we could add a header to the screen, titled "View Source" with an 'x' on the left side. Tapping the 'x' would simply close the screen. - Its a bit hard to tell in the video, but we could make the screen full screen (if its not already) if we add the header. I think overall it'll look much cleaner that way. Let me know if this makes sense or if you have any other questions.
Attachment #8653766 - Flags: ui-review?(jsavory) → ui-review-
Comment on attachment 8653766 [details] View Source demo video Just to add to Jacqueline's feedback. I would make the header white with the grey used in settings for the header text and "x" button.
Attachment #8653766 - Flags: ui-review?(amlee) → ui-review-
Mock for reference to feedback in comment 18.
Thanks for the reviews. I've addressed the issues that you both raised. You can try it out by running attachment 8653198 [details] [review] locally. Note that I made two changes that deviate from the spec provided in attachment 8655524 [details]: 1. I removed the transparency because I was finding that it was getting in the way of reading. 2. The close button ("X") is on the left instead of the right. This is a convention that we've been following for other apps. It's not trivial to change this as the positioning is built into <gaia-header>, so I'd have to patch that.
Attachment #8653761 - Attachment is obsolete: true
Attachment #8653762 - Attachment is obsolete: true
Attachment #8653763 - Attachment is obsolete: true
Attachment #8653764 - Attachment is obsolete: true
Attachment #8653765 - Attachment is obsolete: true
Attachment #8653766 - Attachment is obsolete: true
Attachment #8655905 - Flags: ui-review?(jsavory)
Attachment #8655905 - Flags: ui-review?(amlee)
Comment on attachment 8655905 [details] View Source screenshot Hi, Can you change the header to white? It looks a bit strange that we're using the Settings header with a white background for the code portion. Also please change View source to View Source (capitalized S) to follow our header convention. Thanks!
Attachment #8655905 - Flags: ui-review?(amlee) → ui-review-
Attachment #8655524 - Attachment is obsolete: true
Fixed based on feedback in comment 21. As before, you can try this feature out by building and flashing attachment 8653198 [details] [review] locally.
Comment on attachment 8656708 [details] View Source screenshot Hi Doug, Looks good to me. Thanks!
Attachment #8656708 - Flags: ui-review?(amlee) → ui-review+
Comment on attachment 8656708 [details] View Source screenshot Looks good to me as well!
Attachment #8656708 - Flags: ui-review?(jsavory) → ui-review+
You need to log in before you can comment on or make changes to this bug.