Site Information header in reader mode looks ugly

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: Ehsan, Assigned: ns19041997, Mentored)

Tracking

unspecified
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments)

Posted image Screenshot
See the screenshot, it shows the about:reader?url=foo URL...
Priority: -- → P3

Updated

5 months ago
Assignee: nobody → 1991manish.kumar

Comment 2

5 months ago
(In reply to Manish [:manishkk] from comment #1)
> these lines are code related to this bug?
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> siteIdentity.js#763,779

Line 763 doesn't seem related, as about:reader does not have a EV certificate (hence it does not match the this._isEV check).
What Tim said!
Flags: needinfo?(ehsan)
Mentor: jhofmann

Comment 4

4 months ago
Posted image no ugly header.png

:Ehsan

but same URL working fine for me.

Attachment #9035803 - Flags: review?(ehsan)

Comment 5

4 months ago

(In reply to Manish [:manishkk] from comment #4)

Created attachment 9035803 [details]
no ugly header.png

:Ehsan

but same URL working fine for me.

You need to click the reader icon in the URL bar (the one that looks like a page).

Attachment #9035803 - Flags: review?(ehsan)

Updated

4 months ago
Assignee: 1991manish.kumar → nobody

Some more info on how to solve this bug:

This is how the URL bar figures out the right url to display from an about:reader url: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/urlbarBindings.xml#1065

We should do the same thing in refreshIdentityPopup, e.g. here: https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/browser/base/content/browser-siteIdentity.js#745

It would also be great to have a test for this.

Assignee

Comment 7

3 months ago

I would like to work on this.
Could this bug be assigned to me?
Thanks!

Yup, thank you!

Assignee: nobody → ns19041997
Status: NEW → ASSIGNED
Assignee

Comment 10

3 months ago

Hi, Johann,
I have made a patch regarding the site information header issue.
I also saw the files browser_mixed_content_cert_override.js and browser_check_identity_state.js tests where refreshIdentityPopup() is used.
Could you guide me as to where I should write a test for this.
Thanks!

browser_check_identity_state.js is pretty close to what you want to do, it loads different sites like example.com and checks their security indicators.

You might want to do the same for some URL like about:reader?url=http://example.com, but then we also would like to verify that identity-popup-mainView-panel-header-span contains the right URL (and doesn't contain about:reader).

Does that make sense?

Thanks!

Assignee

Comment 12

3 months ago

Hello Johann,
I have made the requested changes.
Please review.

Hi Neha, thanks for the great contribution.

Before landing this, it would be great if you could do a Try Push. Try is basically Continuous Integration for Firefox, making sure that tests are passing for your patch.

Getting access is outlined here: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

You will need to file a bug and need someone to vouch for you. I'd be happy to do that if you CC me on that bug.

Afterwards, make sure you perform the steps outlined in https://wiki.mozilla.org/ReleaseEngineering/TryServer#Configuration and (most importantly) in https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/auth.html in order to get access on your local command line.

When you have access, to schedule a try run, you can use the handy ./mach try command. A possible syntax for this bug could be:

./mach try -b do -p win64,linux64,macosx64 -u mochitests -t none

(runs all mochitests on Windows, OSX and Linux)

Let me know if you need any help with that!

Assignee

Comment 14

2 months ago

Hi Johann,
I got the access and scheduled a try run.
It has completed.
Please review.
Thanks!

Great, thanks, but I would need the link to the try run to review it :)

Based on searching treeherder for your email address I think it's this one, right? https://treeherder.mozilla.org/#/jobs?repo=try&revision=214a0e0c93b3c65397e121876edad1376af997a3

That looks great to me, feel free to set checkin-needed now!

Assignee

Comment 16

2 months ago

Hello Johann,
yes, this is the one I submitted!

Could you guide me as to how to set the checkin-needed option for this?
I went and searched the treeherder(https://treeherder.mozilla.org/#/jobs?repo=try&revision=214a0e0c93b3c65397e121876edad1376af997a3) page, but couldn't figure out how to set it.

Thanks!

You can do it in this bug, just select "Edit Bug" and modify the "keywords" field to include "checkin-needed" (it should autocomplete it for you).

Assignee

Updated

2 months ago
Keywords: checkin-needed
Assignee

Comment 18

2 months ago

Thanks!
I have made the required changes.
Please review.

Comment 19

2 months ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a14eec92d6e
Improving site Information header in reader mode r=johannh

Keywords: checkin-needed

Comment 20

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.