111524 - Unable to hide/show scrollbars programatically when embedded

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Patrick Griffiths]

I think this is related to: http://bugzilla.mozilla.org/show_bug.cgi?id=67181

The following fails with an assert:

nsCOMPtr<nsIDOMWindow> domWindow;
mWebBrowser->GetContentDOMWindow(getter_AddRefs(domWindow));

nsCOMPtr<nsIDOMBarProp> sb;
domWindow->GetScrollbars(getter_AddRefs(sb));

sb->SetVisible(PR_FALSE);

A quick trace through the SetVisible() code suggests that the problem is a call 
to GlobalWindowImpl::GetWebBrowserChrome() retrieves a null pointer.

This happens in both my code and by inserted the above code into the MFCEmbed 
example.

The full mozilla browser seems to function correct in that in JS, 
window.scrollbars.visible does the right thing.

I'm working with the Mozilla 0.9.5 sources from a tarball drop.

Comment 1

[Johnny Stenback (:jst)]

Dan, can we make this work with our embedding APIs?

Comment 2

[Dan M]

  Current embedding APIs already cover this. The scrollbar object implementation
changes the value of the nsIWebBrowserChrome::chromeFlags attribute. Maybe it's
not well understood that that's a live value; embedding clients are supposed to
respond when it changes.
  I don't see the problem reported by Patrick. Using some JS in a page loaded in
mfcembed, GlobalWindowImpl::GetWebBrowserChrome is succeeding for me. It
eventually fails in CBrowserImpl::GetChromeFlags, which is unimplemented and
returns an error, aborting the attempt to change the attribute's value. That's
just an implementation detail of mfcembed; it's a showcase for many things but
apparently not for how the chromeFlags attribute should be hooked up.
  Just guessing here. You'd get a null pointer back from
GlobalWindowImpl::GetWebBrowserChrome if you didn't set the containerWindow
attribute of your nsIWebBrowser object. But mfcEmbed is doing that and is
working for me. Up to the point where chromeFlags aren't hooked up. Finger
hovering over the "close invalid" button...

Comment 3

[Dan M]

s/invalid/works for me/

Comment 4

[Patrick Griffiths]

Implementing the Set/GetChromeFlags in the obvious way, with the chrome flags 
initialized to CHROME_DEFAULT only has the effect of preventing the assertion 
that occurs otherwise (not suprising). The scrollbars still remain.

Am I incorrect in assuming that calling SetVisible() as I described previously 
should simply flip the scollbar off without serious intervention from the 
embedding application? After all, the scrollbar is present by default when we 
fire-up embedded Mozilla, so it's not as if the embeddor was responsible for 
drawing and managing the scrollbars... And, none of the other DOM interfaces 
presented to us require any serious hocus-pocus to use.

As for how the chromeFlags attribute should be hooked up, an embedding 
application is theoretically limited to a restricted set of public interfaces:

http://www.mozilla.org/projects/embedding/apiReviewNotes.html

Even considering that this is all very much in a state of flux, how are 
embeddors supposed to remain (mostly) in the confines of the public API's and 
be able to show/hide the scrollbars?

Comment 5

[Dan M]

You're talking about the other side; how to implement the code in the embedding
app that controls the scrollbars in the embedded browser, yes? Something like
this should work in, e.g. mfcEmbed:

nsresult CBrowserImpl::SetChromeFlags(PRUint32 aChromeMask)
{
  // merge chromeFlags somehow with their stored copy in m_pBrowserFrameGlue
  nsCOMPtr<nsIScrollable> scrolldoc(do_QueryInterface(mWebBrowser));
  if (scrolldoc) {
    long prefValue = aChromeMask & nsIWebBrowserChrome::CHROME_SCROLLBARS ?
                     NS_STYLE_OVERFLOW_AUTO : NS_STYLE_OVERFLOW_HIDDEN;
    scrolldoc->SetDefaultScrollbarPreferences
               (nsIScrollable::ScrollOrientation_Y, prefValue);
    scrolldoc->SetDefaultScrollbarPreferences
               (nsIScrollable::ScrollOrientation_X, prefValue);
  }
  return NS_OK;
}

cribbing from Mozilla. If that actually worked, I still claim we'd be covered as
far as APIs go. Unfortunately that code seems to have regressed. It has no
effect, even in Mozilla.

Patrick, do you agree that barring any API change that may be necessary to fix
the regression, we do have sufficient API coverage for embeddors to implement
scrollbar visibility? Or am I still not catching on?

Comment 6

[Patrick Griffiths]

To answer the question: a qualified yes.

The nsIScollable method actually works if one calls them before loading the 
document. The problem I'm having is doing it dynamically after the document has 
loaded without also doing a reload.

In js if one sets window.scrollbars.visible = false in an onmouseover event 
(for example), then to get the scrollbars to go away one needs to click the 
reload button. Same thing happens for embedding; I have to reload the page for 
the visibility change to take effect. Is this the intention? If it is, then the 
problem becomes an API consistency one because the comments for nsIScrollable 
from the embedding API review notes I mentioned previously are:

"Don't want to expose this interface. Similar functionality available in 
nsIDOMWindow."

At this point, that's problematic for embeddors. Is this an embedding problem 
or a DOM problem:

- should nsBarProps::SetVisible function without intervention from the 
embedding app?

or ....

- should nsIScrollable be public.

Comment 7

[Dan M]

Oh. Heh. I thought nsIScrollable was public. I see the problem. Looking into it.
Thanks for finding this.

Comment 8

[Dan M]

Attachment: implement scrollbar attribute internally, without exposing nsIScrollable to embedding

As discussed above, current unpatched code, requested to change scrollbar
visibility, calls up to the embedding window, which must then find some way of
implementing the attribute in its main content window. Unfortunately, there is
no published embedding interface for doing this. This patch implements
scrollbar visibility directly, obviating the need for embedding apps to be
involved and therefore the need for a new interface.

The new scheme does fix, or at least take a stand, on an issue with the
applicability of the historical scrollbar chrome flag to the more modern world
in which a single chrome window can contain multiple browsers. See comments in
the patch for details.

Comment 9

[Dan M]

(Oh -- and note that the above patch still doesn't exactly work. It works as
well as the current code, which is to say that neither the current or patched
code is actually affected by our feeble attempts to change the visibility of a
docshell's scrollbar. Bug 54167 may address this. This patch pretty much just
promises to deal with the issue internally, and take a stand on the embedding API.)

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 69734 [details] [diff] [review]
implement scrollbar attribute internally, without exposing nsIScrollable to embedding

sr=jst

Comment 11

[Dan M]

.