Opened 8 years ago

Closed 8 years ago

#13785 closed enhancement (fixed)

Increase wxWebView customization

Reported by: Allonii Owned by:
Priority: low Milestone:
Component: wxMSW Version: stable-latest
Keywords: wxWebview, 3d border, menu, hotkeys, accelerators Cc:
Blocked By: #13784, #13784, #13784, #13784, #13784, #13784, #13784 Blocking:
Patch: yes

Description

I have made some fixes to wxWebViewIE to behave more compatible with other wxWebView instances.

The changes,

  • No 3d border.
  • Disable the forced hotkeys for instance CTRL-N ect.
  • Disable the default context menu.

All these changes are only possibly if my other patch #13784 gets accepted since it uses the IDocHostUIHandler.

These changes are only small changes with #13784 one can do more things, for instance

  • Control drag and drop
  • Extend DOM
  • Change user interaction

ect

Attachments (2)

web.patch download (6.4 KB) - added by Allonii 8 years ago.
fixed ref count
web2.patch download (11.3 KB) - added by Allonii 8 years ago.
the new patch

Download all attachments as: .zip

Change History (18)

comment:1 follow-up: Changed 8 years ago by vadz

  • Milestone 2.9.4 deleted
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from wxWebView compatibility fixes to Increase wxWebView customization

This seems like a rather big amount of code to just get rid of 3D border...

And the rest of the changes don't look very useful to me: if we really want to disable the standard accelerators, we should at least use our own wxAcceleratorTable instead of just not having any accelerators at all. And what's wrong with the default context menu?

Drag and drop could be useful to have though so this might be still nice to have for the possible future enhancements.

At lower level, where is m_refCount initialized? And why are not the macros from wx/msw/ole/oleutils.h used to implement IUnknown part? The macros are not particularly nice but at least this would make the semantics known because consistent with the rest of the code. As it is, I don't know if the new object is supposed to start with ref count of 0 or 1 and this is rather important to know.

Finally, from the style point of view it would be really better to use static_cast<> instead of C style casts.

comment:2 Changed 8 years ago by vadz

  • Blocked By 13784 added

(In #13784) Could this be done in wxWebView itself? I just don't see why should it be provided in wxActiveXContainer if it can only ever be used with IWebBrowser2?

comment:3 Changed 8 years ago by Allonii

  • Blocked By

(In #13784) Replying to vadz:

Could this be done in wxWebView itself?

That's the problem this can only be done in wxActiveXContainer what I can see since wxWebViewIE uses the wxActiveXContainer which in turn uses IOleClientSite. So in other words to set IDocHostUIHandler in the correct way one should use the IOleClientSite. See http://msdn.microsoft.com/en-us/library/aa770041%28v=vs.85%29.aspx#IDocHostUIHandler for more info.

I just don't see why should it be provided in wxActiveXContainer if it can only ever be used with IWebBrowser2?

A quote from Microsoft's msdn

"The external IDocHostUIHandlerDispatch interface is used by controls that query the host's site for the IDocHostUIHandlerDispatch interface. The WebBrowser control is one control that does this."

So they say there should be more controls using this handler. By they way on the first page of wxActiveXContainer documentation it says

"It is somewhat similar to the ATL class CAxWindow in operation."

And CAxWindow provides a SetExternalUIHandler method to set the IDocHostUIHandler.

In the end I think we will need to implement this to be able to customize the web browser control.

comment:4 follow-up: Changed 8 years ago by vadz

  • Blocked By

(In #13784) > So in other words to set IDocHostUIHandler in the correct way one should use the IOleClientSite.

Is there anything wrong with just providing an accessor to m_frameSite and allowing wxWebView to do the rest?

comment:5 in reply to: ↑ 1 Changed 8 years ago by Allonii

  • Blocked By

Replying to vadz:

This seems like a rather big amount of code to just get rid of 3D border...

If you know another way be my guest.

And the rest of the changes don't look very useful to me: if we really want to disable the standard accelerators, we should at least use our own wxAcceleratorTable instead of just not having any accelerators at all. And what's wrong with the default context menu?

I did not say anything about disabling all the accelerators but only the ones that are forced upon us. The translate accelerator method in IDocHostUIHandler is called AFTER the users accelerator table is called (depending on wxActiveXContainer). So this change will only affect people without an accelerator table and don't want internet explorer to pop up when CTRL-N is pressed or the search dialog on CTRL-F for that matter.

There is nothing wrong with the default context menu but not all people want to default context menu to be shown (For instance showing a custom html page). The dev should be given a choice at least, perhaps he wants to show a custom menu. I disabled the context menu with the thought that it did not show up on other platforms but I have not tested it since I only have window.

At lower level, where is m_refCount initialized? And why are not the macros from wx/msw/ole/oleutils.h used to implement IUnknown part? The macros are not particularly nice but at least this would make the semantics known because consistent with the rest of the code. As it is, I don't know if the new object is supposed to start with ref count of 0 or 1 and this is rather important to know.

Finally, from the style point of view it would be really better to use static_cast<> instead of C style casts.

I forgot about m_refCount sorry it should be 0. I followed the implementation of the 'IUnknown' interface from '\include\wx\msw\webview_ie.h' the classes wxIInternetBindInfo, wxIInternetProtocolSink, wxIInternetSession ect. But sure I think the macros should also do.

Sorry but which casts are you talking about? Nearly all the casts in 'activex.cpp' and "webview_ie.cpp" are C style casts.

comment:6 in reply to: ↑ 4 Changed 8 years ago by Allonii

Replying to vadz:

(In #13784) > So in other words to set IDocHostUIHandler in the correct way one should use the IOleClientSite.

According to msdn yes. The control will request the IDocHostUIHandler from IOleClientSite e.i. "host's site" and that's located in FrameSite.

Is there anything wrong with just providing an accessor to m_frameSite and allowing wxWebView to do the rest?

Sorry don't understand, what do you mean?

Changed 8 years ago by Allonii

fixed ref count

comment:7 follow-up: Changed 8 years ago by vadz

  • Blocked By

(In #13784) > Sorry don't understand, what do you mean?

I meant that wxWebView could "inject" or otherwise customize IOleClientSite implementation used by wxActiveXContainer. But in fact I guess it would be wiser to provide some hook into IOleClientSite::QueryInterface() as it can probably be queried for more than just this interface. I.e. I'd add some virtual wxActiveXContainer::QueryClientSiteInterface() and then override it in wxIEContainer that would be defined in webview_ie.cpp.

What do you think?

comment:7 follow-up: Changed 8 years ago by vadz

  • Blocked By

I don't want to belittle your work, I just wonder if getting rid of 3D border is sufficiently important to motivate adding all this code.

As for the accelerators, I didn't understand how this worked, thanks for the explanation.

For the menu, I think the point is that it looks a potentially useful feature and it's not obvious that never showing it is really better than always showing it. Ideal would be to make this configurable but I don't know what is the state under the other platforms and this would be important for this decision.

comment:8 in reply to: ↑ 7 Changed 8 years ago by Allonii

Replying to vadz:

I don't want to belittle your work, I just wonder if getting rid of 3D border is sufficiently important to motivate adding all this code.

Don't worry I did neither intend to be harsh =). I agree its a lot of code but there are mainly two reasons for this,

  1. Nearly all of the code in 'activex.h' is defined in 'mshtmhst.h' but there is a lot of compiler inconsistency although 'mshtmhst.h' has been around since IE 4.0. I though the best way to solve this is to define our own IDocHostUIHandler and its structs. For instance on my computer VC++ 2008 has 'mshtmhst.h' but mingw 3 does not.
  2. All of IDocHostUIHandler functions are pure virtual and that's why one needs to implement all of them and not only the ones we are interested of.

The whole idea here is not about the 3D border (although it is one of the issues) but about the ability to customize the web browser control. There is more one can do with IDocHostUIHandler not mentioned here. So basically I'm trying to build up a base for further customizations.

For the menu, I think the point is that it looks a potentially useful feature and it's not obvious that never showing it is really better than always showing it. Ideal would be to make this configurable but I don't know what is the state under the other platforms and this would be important for this decision.

Perhaps we can get someone using wxWebView on other platforms opinion here first. But anyway we could leave this one out now until someone replays. But I think that the 3d border and accelerators needs to be changed.

comment:9 Changed 8 years ago by Allonii

  • Blocked By

(In #13784) Replying to vadz:

I meant that wxWebView could "inject" or otherwise customize IOleClientSite implementation used by wxActiveXContainer. But in fact I guess it would be wiser to provide some hook into IOleClientSite::QueryInterface() as it can probably be queried for more than just this interface. I.e. I'd add some virtual wxActiveXContainer::QueryClientSiteInterface() and then override it in wxIEContainer that would be defined in webview_ie.cpp.

What do you think?

Without looking into the code the first thing I come to think of is the possibility of multiple definitions (IDocHostUIHandler and it's structs). This might be a problem since wxWebView might have and definition, the dev might have one and if we intend on using IDocHostUIHandler somewhere else then we might need to redefine it again.

But you have been doing this for a longer time then me, so if you still think this might be a better approach then replay here and I will try to implement it as soon as possible.

comment:10 Changed 8 years ago by vadz

  • Blocked By

(In #13784) We could provide a separate header with IDocHostUIHandler declaration for the compilers/environments that don't have it already so I don't really see how is this a problem.

As for "better approach": of course, in the grand scheme of things both approaches are quite equivalent. But I do think that keeping related things (IDocHostUIHandler and wxWebView) together and, especially, not necessarily related things (IDocHostUIHandler and wxActiveXContainer) separately is better.

comment:11 Changed 8 years ago by Allonii

  • Blocked By

(In #13784) Ok now I have made the changes.

The definition of IDocHostUIHandler is no longer in activex. I have Implemented and documented the method QueryClientSiteInterface to enable devs to override this method and supply their own interfaces. This should enable them to supply other interfaces rather then just IDocHostUIHandler.

I think that's how you imagined this?

comment:13 Changed 8 years ago by Allonii

Fixed this patch to reflect #13784 changes. And defined IDocHostUIHandler just like the other definitions in 'webview_ie' (wxIInternetBindInfo, wxIInternetProtocolSink ect). I have also changed to make more use of 'oleutils.h' and to not disable the default context menu.

Changed 8 years ago by Allonii

the new patch

comment:14 Changed 8 years ago by steve_lamerton

  • Blocked By

(In #13784) This patch looks good to me, if there are no objections I will commit this and #13785 with a few minor changes this weekend.

comment:15 Changed 8 years ago by SJL

  • Blocked By

(In #13784) (In [70361]) Add wxActiveXContainer::QueryClientSiteInterface to allow customisation of ActiveX controls.

Closes #13784.

comment:16 Changed 8 years ago by steve_lamerton

  • Resolution set to fixed
  • Status changed from confirmed to closed

Committed as r70362. I credited you in changes.txt as Allonii, if you would like me change it just let me know. I did leave the context menu enabled as GTK also has one and enabled themes as this makes it act more like native IE.

Note: See TracTickets for help on using tickets.