Stop the social sidebar from interfering w/ awesomebar and NewTab

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: markh)

Tracking

17 Branch
Firefox 17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

On my New Tab page, I'm seeing a link to the same location as that of the sidebar page. We probably want to make sure that the sidebar doesn't affect the New Tab page specifically, and likely the awesomebar, etc.
(Assignee)

Comment 1

5 years ago
and also "History" (where it also appears).  Hopefully this will be as simple as adding disableglobalhistory="true" to our <browser> elements...
(Assignee)

Comment 2

5 years ago
Created attachment 653278 [details] [diff] [review]
Prevent sidebar from being added to global history

This patch really just adds disableglobalhistory="true" to the sidebar window - the rests are all tests for this, including testing that the toolbar panels also don't end up in history - they weren't, but I figured the tests are worth keeping.
Assignee: nobody → mhammond
Attachment #653278 - Flags: review?(jaws)
Seems like we'll need to do something similar for the social panels, right? Those are just iframes so it may be a bit more complicated...

(Why use e.data.data.location instead of just e.data.location, for got-panel-message?)
Attachment #653278 - Attachment is patch: true
Comment on attachment 653278 [details] [diff] [review]
Prevent sidebar from being added to global history

Review of attachment 653278 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, but can you file another bug for the panels, service windows, and chatboxes? Since at least panels and chatboxes use xul:iframes, we'll want to add the disableglobalhistory attribute to xul:iframe.

The service window can get the disableglobalhistory attribute on its embedded browser.

r+ with the below change.

::: browser/base/content/test/social_worker.js
@@ +39,5 @@
>          if (testPort && event.data.result == "ok")
> +          testPort.postMessage({topic:"got-panel-message",
> +                                data: {
> +                                  location: event.data.location
> +                                }

Should just be {topic:"...", location: event.data.location} as Gavin suggested.
Attachment #653278 - Flags: review?(jaws) → review+
Summary: Avoid interfering w/ awesomebar and NewTab → Stop the social sidebar from interfering w/ awesomebar and NewTab
(Assignee)

Comment 5

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Seems like we'll need to do something similar for the social panels, right?
> Those are just iframes so it may be a bit more complicated...

I expected to need to change the panels, but the test attempts to verify that the content in the panels doesn't end up in history.  I should revisit that and check it is doing what I think it is.  Certainly the "service window" and chat windows will need to be considered.

> (Why use e.data.data.location instead of just e.data.location, for
> got-panel-message?)

*shrug* - we generally seem to use a convention of using just 2 elements - data.topic and data.data - but yeah, as the test already used data.result, I probably should have just ignored that convention in this case.
(Assignee)

Comment 6

5 years ago
Created attachment 653625 [details] [diff] [review]
Now handles service window too

This new patch:

* Ensures the service window URL doesn't end up in global history (it did).

* Doesn't use 'event.data.data.location' as requested.

I also manually verified that the "panels" don't end up in history - they never did, so this patch takes no action to avoid it, but does test it.

Re-requesting review so:

* The (new) code to prevent the service window being stored in history can be checked.
* You can confirm the test in place for the panels is appropriate, so no further followup on that is necessary.

I also opened bug 784238 to ensure the chat windows don't end up in history once they land.
Attachment #653278 - Attachment is obsolete: true
Attachment #653625 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

5 years ago
Created attachment 653628 [details] [diff] [review]
Updated - gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell

Gavin pointed out in IRC that gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell, so updating patch accordingly...
Attachment #653625 - Attachment is obsolete: true
Attachment #653625 - Flags: review?(gavin.sharp)
Attachment #653628 - Flags: review?(gavin.sharp)
Comment on attachment 653628 [details] [diff] [review]
Updated - gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell

(You should bump up the context in exported patch to have 8 lines of context, helps for reviewing: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3F )

IIRC we require that all manifest URIs be absolute, so I'm not sure that ensureSocialUrlNotRemembered needs an origin parameter?
Attachment #653628 - Flags: review?(gavin.sharp) → review+
We'll probably also need to do something similar for the social panels, right?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> We'll probably also need to do something similar for the social panels,
> right?

We discussed this on IRC - those are just iframes, and so don't have global history enabled by default, so no need to do anything there.
(Assignee)

Comment 11

5 years ago
Created attachment 654528 [details] [diff] [review]
minor changes to tests

This new version of the patch removes the 'origin' arg from ensureSocialUrlNotRemembered but is otherwise identical, so I'll carry the r+ forward.  Also, FWIW, this patch is part of the try run at https://tbpl.mozilla.org/?tree=Try&rev=1e0b679f0a6b
Attachment #653628 - Attachment is obsolete: true
Attachment #654528 - Flags: review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9068173a7da
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c9068173a7da
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
I'm seeing odd failures to add to history on m-c in a build made today; for example selecting a full build log in tbpl it didn't push a history frame (i.e. back remained disabled).  This is the only "interesting" checkin recently in the area
An updated build seems to have fixed this; never mind.
Duplicate of this bug: 783318
You need to log in before you can comment on or make changes to this bug.