128659 - keys don't work in content area

Status: VERIFIED FIXED

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

Description

[Christopher Blizzard (:blizzard)]

Steps to reproduce:

o start the browser
o load a site from bookmarks
o you can't scroll the content area using the keys

There's also an error in the JS console:

uncaught exception: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIDOMWindowInternal.focus]"...

Backing out the checkin to nsGlobalWindow.cpp fixes this.  Please either fix it,
or back it out with the associated checkins.

Comment 1

[R.K.Aa.]

can't navigate with arrow keys in a form either.

Comment 2

[Christopher Blizzard (:blizzard)]

This is on the 0.9.9 branch, too.

Comment 3

[Sivakiran Tummala]

changing the priority to P1

Comment 4

[Christopher Blizzard (:blizzard)]

I backed this out of 0.9.9.

Comment 5

[Jean-Francois Ducarroz]

The function widget/src/gtk/nsIWidget::IsEnabled return false everytime we try
to set the focus on the window! This because mWidget is null and the default
return value is false. If I return true when mWidget is null fix the problem.

Any advise from a gtk specialist would be appreciate...

Comment 6

[Christopher Blizzard (:blizzard)]

That code hasn't been written for gtk at all.  That code has never worked.

Comment 7

[R.K.Aa.]

bug 128726, bug 128737 and bug 128729 all seem to be cured by backing out v1.492
of nsGlobalWindow.cpp

Comment 8

[Carsten Menke]

I could confirm Comment #1 with today's trunk build ( March 3rd 2002 ). When I
wanted to fill a bugreport with that build I couldn't use the lef and right
arrow keys as well as copy & past filled always the url bar but not the form field.

Build on Linux Mandrake 8.0 gcc-2.95.3 gtk-1.2.10

Comment 9

[Jean-Francois Ducarroz]

Christopher, which code do you mean, the nsIWidget::IsEnabe?

Comment 10

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

This patch will avoid the function IsEnable to return false when the widget is
not really disable.

Comment 11

[Jean-Francois Ducarroz]

I really need to fix this problem without backing out my change in
nsGlobalWIndows.cpp which is needed to avoid problem with the message compose
window and the mailnews print window. I am currently using a trunk build with
that patch to submit those comment and for me it works fine. WOuld be nice if
somebody could review it...

Comment 12

[Jean-Francois Ducarroz]

I should have said:... a trunk linux build...

Comment 13

[Jean-Francois Ducarroz]

*** Bug 128729 has been marked as a duplicate of this bug. ***

Comment 14

[Jean-Francois Ducarroz]

*** Bug 128726 has been marked as a duplicate of this bug. ***

Comment 15

[Jean-Francois Ducarroz]

*** Bug 128737 has been marked as a duplicate of this bug. ***

Comment 16

[Jean-Francois Ducarroz]

I have also verified that the patch fix all the dup bugs

Comment 17

[R.K.Aa.]

ditto. built with the patch, tested all issues mentioned in the dups: All works.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 72329 [details] [diff] [review]
Proposed fix, v1

>-  *aState = mWidget && GTK_WIDGET_SENSITIVE(mWidget) ? PR_TRUE : PR_FALSE;
>+  if (mWidget)
>+    *aState = GTK_WIDGET_SENSITIVE(mWidget) ? PR_TRUE : PR_FALSE;
>+  else
>+    *aState = PR_TRUE;

How about just:

  *aState = !mWidget || GTK_WIDGET_SENSITIVE(mWidget);

(C++ 5.15 says that || and && yield a boolean, which is
implicitly converted to 0 or 1 because of section 4.7,
clause 4.  Are there compilers where this doesn't work
right?)

I'd be interested to hear what danm and blizzard think
of the idea, though (see bug 126786).

Comment 19

[Jean-Francois Ducarroz]

dbaron, I am fine with your style change. All I need at this point is a linux
guy that can give me a R=. But if not body find a problem with that patch until
tonight, I'll probably check it in anyway before tomorrow morning build.

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 128759 has been marked as a duplicate of this bug. ***

Comment 21

[Brian Ryner (not reading)]

blizzard, should we be doing something else entirely if the widget doesn't have
an mWidget?

Comment 22

[Jean-Francois Ducarroz]

I took a close look at danm implementation of isEnable and GetEnable done for
bug 126786 on which my work was based on. I found some inconsistence in the fact
that sometime those function return true or false as default value whatever if
the function was fully or not implemented. Sofar problem show up only on linux
but we might have problem as well on BeOS and Cocoa.

Before danm implements nsIWidgetIsEnable, we weren't able to query if a widget
was enable or not. We just presumed widget were always enable. Therefore, if we
want to not break that assumption, we must still say a widget is enable by
default unless we now it's realy disabled.

If we cannot determine if a widget is enable or disable, the function IsEnable
must return TRUE and depending of the case an error code. This is not the case
for the windows, beos, linux and cocoa implementation (but doesn't seems to
cause any harm on Windows). I'll post a patch that cleanup all that...

Comment 23

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2

Comment 24

[Jean-Francois Ducarroz]

danm, please review.

Comment 25

[Christopher Blizzard (:blizzard)]

-  *aState = mWidget && GTK_WIDGET_SENSITIVE(mWidget) ? PR_TRUE : PR_FALSE;
+  *aState = !mWidget || GTK_WIDGET_SENSITIVE(mWidget);

mWidget is used by exactly one widget right now, the scrollbar.  Do we ever
disable scrollbars?  I would just suggest not even checking that.  I'm going to
have to hack on this soon, anyway so I'm just going to replace any placeholders.

Comment 26

[Jean-Francois Ducarroz]

If I coreclty understand your comment, you are suggesting me to just always set
aState to true? But then, why to we have code to disable the widget (scrollbar)
in the function nsIWidget::Enable?

I think my fix would be more consistent with the actual code even if it's kind
of no sense!

Comment 27

[Christopher Blizzard (:blizzard)]

If you want to leave it as is, that's fine, even though it's not going to be
used and I'm going to go in there with a hammer and chisel sometime very soon.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 72357 [details] [diff] [review]
Proposed fix, v2

r=bryner

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 128824 has been marked as a duplicate of this bug. ***

Comment 30

[Jean-Francois Ducarroz]

I have backed out my previous checkin for nsGlobalWindow.cpp which expose this
problem as I wasn't able to get the needed review and approval on time before
today's verification build. I'll move the proposed fix for the underline problem
into bug 109081 which is now reopen.

Comment 31

[Jonathan Abbey]

This bug is still present in the 2002030406 linux build.

Comment 32

[Jean-Francois Ducarroz]

Right, the fix miss that build, next one will be ok...

Comment 33

[Christopher Blizzard (:blizzard)]

Comment on attachment 72357 [details] [diff] [review]
Proposed fix, v2

sr=blizzard

Comment 34

[Jean-Francois Ducarroz]

Thanks Christopher, I'll report your SR in bug 109081 where the patch now lives...

Comment 35

[Boris Zbarsky [:bzbarsky]]

*** Bug 128881 has been marked as a duplicate of this bug. ***

Comment 36

[Jean-Francois Ducarroz]

*** Bug 128885 has been marked as a duplicate of this bug. ***

Comment 37

[Jean-Francois Ducarroz]

*** Bug 128882 has been marked as a duplicate of this bug. ***

Comment 38

[Adam D. Moss]

Ah, I think that this back-out also fixed the problem
I was having where none of my sidebar bookmark links
were clickable.  Better now.

Comment 39

[Alfonso Martinez]

*** Bug 128913 has been marked as a duplicate of this bug. ***

Comment 40

[Boris Zbarsky [:bzbarsky]]

*** Bug 129048 has been marked as a duplicate of this bug. ***

Comment 41

[Boris Zbarsky [:bzbarsky]]

*** Bug 129043 has been marked as a duplicate of this bug. ***

Comment 42

[Tracy Walker [:tracy]]

verified on linux build 2002-03-05-11-trunk