Closed Bug 1432090 Opened 6 years ago Closed 2 years ago

[CSD] Button layout not according to settings

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
96 Branch
Tracking Flags:
Tracking Status
firefox96 --- fixed

People

(Reporter: chalisesagar, Assigned: smurfd)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

Bug 1432090 - [CSD] Button layout not according to settings - Widget background work r=emilio
47 bytes, text/x-phabricator-request
Details | Review
Bug 1432090 - Honor GTK button layout. r=stransky
48 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

The CSD settings on current developer edition doesnot respect button layout. When drawn in titlebar the button layout are not as they should be.


Actual results:

My system button layout is "close:maximize" but the layout for firefox is ":maximize,close"


Expected results:

The button layout should respect the settings from dconf.
Component: Untriaged → Widget: Gtk
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
This affects me as well. I use "mac osx" layout but FF just dumps the buttons in the top right corner.

Yeah, notised this today aswell.

Hope its okey to start looking at it, ie assign it to myself.
Im guessing it should be somewhere in widget/gtk/nsWindow.cpp ...

Assignee: nobody → smurfd
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

My button order is :
Close, minimize, maximize, <title>

instead of the standard :
<title>, minimize, maximize, close

In release, ie now 65.0 this still shows the standard button layout, even without me having it that way (ie this bug :)).
If i try Developer or Nightly though, the button order is as it should be according to my GNOME settings.
...

Well, tried to change button order to :
Close, maximize, minimize, <title>
.. and restart Nightly.
It would still stick to :
Close, minimize, maximize, <title>

Will try to find the patch that rearranged the sides of the buttons, that would be in the right ballpark of where this change should be done.

Hey Jimm,

I might have a suggestion to fix this. Not sure if i should needinfo you or Martin Stransky (i see he has done a ton of GTK work)
What if we, instead of https://searchfox.org/mozilla-central/source/widget/gtk/gtk3drawing.cpp#410-442
Do so that we tokenize the decorationLayout string, into the actual button strings.
Or rather 1st we tokenize the string into two strings. Before and after the ':'
Then we do another round of tokenization of both before and after the ':' looking for ','

So if we have : close,maximize,minimize:menu

1st we'd get "close,maximize,minimize" and "menu"
2nd we'd get "close", "maximize", "minimize" and "menu"

That way the need of keeping track of reverse order or not is not needed. we would look for the actual buttons instead.

BUT, i guess there is a reason that it has not been done before?

Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies) → needinfo?(stransky)

(In reply to Nicklas Boman [:smurfd] from comment #4)

BUT, i guess there is a reason that it has not been done before?

GetGtkHeaderBarButtonLayout() can be fixed for custom button layout but the main task is to propagate the button order to browser layout [1]. You need to use -moz-box-ordinal-group to re-arrange the buttons and @media glue to get the info from toolkit.

[1] https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/themes/linux/browser.css#719

Flags: needinfo?(stransky)

(In reply to Martin Stránský [:stransky] from comment #5)

GetGtkHeaderBarButtonLayout() can be fixed for custom button layout but the main task is to propagate the button order to browser layout [1]. You need to use -moz-box-ordinal-group to re-arrange the buttons and @media glue to get the info from toolkit.

[1] https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/themes/linux/browser.css#719
Im not sure i follow.

I tried todo something like :
@media (-moz-gtk-csd-reversed-placement) {
.titlebar-close,
.titlebar-max,
.titlebar-min,
.titlebar-buttonbox-container {
-moz-box-ordinal-group: 0;
},

and tried to arrange the order of the buttons. But it would not follow the arrangements i tried.
As i write this, i think that maby i need to do something like (will try this shortly!) :

@media (-moz-gtk-csd-reversed-placement) {
.titlebar-close {-moz-box-ordinal-group: 3;},
.titlebar-max {-moz-box-ordinal-group: 2;},
.titlebar-min {-moz-box-ordinal-group: 1;},
.titlebar-buttonbox-container {
-moz-box-ordinal-group: 0;
},

But still, that would only be a Static solution.
Is not the idea, to read what is set in dconf, and then apply That order to the window buttons?
(IF that is possible, or endgame, to Make that possible)

Flags: needinfo?(stransky)

I think the params for -moz-box-ordinal-group needs to be exported from widget/gtk3 as media values and used in the css.

See this code for inspiration [1] and how is the mCSDCloseButton value exported to JS for instance. You'd need to pass button order from GetGtkHeaderBarButtonLayout() to the js/css so you'd need a new mCSDCloseButtonPosition value which holds the ordinal-group value, export it as atom/media (as well as we do for mCSDCloseButton) and pass it to the appropriate -moz-box-ordinal-group.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/widget/gtk/nsLookAndFeel.cpp#1163

Flags: needinfo?(stransky)

btw. output of GetGtkHeaderBarButtonLayout() is already sorted according to the system setting so you just need to read the button array and create correct mCSD*ButtonPosition values.

There is something what i guess is trivial that i'm missing.

As you say, the output of GetGtkHeaderBarButtonLayout() is following the order that is set in the system settings.

So i looked at https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout/Ordering_Flex_Items and fiddled with the code example. https://mdn.github.io/css-examples/flexbox/order/order.html#
There if i change the number of the order value it will move the buttons.

Then since i have my buttons ordered as close,maximum,minimum:menu it hits the -moz-gtk-csd-reversed-placement
I figured id play around with button arrangements in the .css file manually to do as i said. But no...

Went from :
@media (-moz-gtk-csd-reversed-placement) {
.titlebar-buttonbox-container,
.titlebar-close {
-moz-box-ordinal-group: 0;
}
}
To something like (one of many-many-many attempts) :
@media (-moz-gtk-csd-reversed-placement) {
.titlebar-buttonbox-container,
.titlebar-close :nth-child(1) {
-moz-box-ordinal-group: 0;
order: 1;
},
.titlebar-max :nth-child(2) {
-moz-box-ordinal-group: 0;
order: 2;
},
.titlebar-min :nth-child(3) {
-moz-box-ordinal-group: 0;
order: 3;
}
}

It rearranged the buttons, but not to what i wanted. It became minimum,maximum,close... Also tried to move the Close button to the middle via setting the order value. No go.

Have also made preparations for a -moz-gtk-csd-custom-placement just in case that is needed, while i was trying to figure out how things where working.
I feel that if i can't manually rearrange the buttons, how would i get it to work per automagic? :)
As i started with, it is something that im missing. or rather i think its the CSS that confuses me. It creates sortof a black layer between the actual gtk painting of the button, from the array of the button layout... Kindof the whole point of this :)

So if you could lend a hand would be great :)

Flags: needinfo?(stransky)

AFAIK the -moz-box-ordinal-group determines the relative position so you'd need something like:

.titlebar-close {
  -moz-box-ordinal-group: 0;
},
.titlebar-max {
  -moz-box-ordinal-group: 500;
},
.titlebar-min {
  -moz-box-ordinal-group: 1000;
}

to sort them out. I'm not sure how is the ordinal-group sorted - if bigger comes first or last - you'd need to try.

Flags: needinfo?(stransky)

Okay so after many ifs and butts, i now know how this works (atleast manually)

In this https://searchfox.org/mozilla-central/source/browser/themes/linux/browser.css#659
you control wether the buttonbox is on the left or the right of the window title (or the bar of tabs if you have flipped the switch to not show the titlebar in Customize mode)
-moz-box-ordinal-group: 0; = left
-moz-box-ordinal-group: 1; = right
(It needs to be added to the .titlebar-buttonbox-container, not to the .titlebar-close as it is now)

Here https://searchfox.org/mozilla-central/source/browser/themes/linux/browser.css#624,635,649
If we add -moz-box-ordinal-group: 0, -moz-box-ordinal-group: 1, -moz-box-ordinal-group: 2
0 is always left, 1 is middle and 2 is the right, always. Does not matter if you have the buttonbox to the left or to the right of the window title.

... now we "just" need to modify these -moz-box-ordinal-group:s dynamicly ...

(In reply to Nicklas Boman [:smurfd] from comment #11)

... now we "just" need to modify these -moz-box-ordinal-group:s dynamicly ...

see my comment 7 - you can export the ordinal number for each button by @media.

Attached patch Bug1432090_190515_2210.patch (obsolete) — — Splinter Review

Hope its okay that i have attached a sortof progress patch.
Will definitively use Phabricator once im done.

Main issue, the value of the button positions does not transfer as it should to the browser.css
(If i set the parameters manually in the css like :
--moz-gtk-csd-close-button-position: 0;
--moz-gtk-csd-maximize-button-position: 1;
--moz-gtk-csd-minimize-button-position: 2; it all works...)
Which has been kindof the problem all along.

But, it seems though that since a while ago, the button order is not read correctly from what is set in Dconf(i think). So i made a thing in gtk3drawing.cpp

But then again, its probably something simple that im not following...

Flags: needinfo?(stransky)

(In reply to Nicklas Boman [:smurfd] from comment #13)

Created attachment 9065211 [details] [diff] [review]
Bug1432090_190515_2210.patch

Hope its okay that i have attached a sortof progress patch.
Will definitively use Phabricator once im done.

Main issue, the value of the button positions does not transfer as it should
to the browser.css
(If i set the parameters manually in the css like :
--moz-gtk-csd-close-button-position: 0;
--moz-gtk-csd-maximize-button-position: 1;
--moz-gtk-csd-minimize-button-position: 2; it all works...)
Which has been kindof the problem all along.

But, it seems though that since a while ago, the button order is not read
correctly from what is set in Dconf(i think). So i made a thing in
gtk3drawing.cpp

But then again, its probably something simple that im not following...

You can directly debug that in Firefox via. Browser Toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox)

Flags: needinfo?(stransky)
Attached patch Bug1432090_190614_2300.patch (obsolete) — — Splinter Review

I have been poking at this to and from ... was close to giving up, but then yesterday after starting over... i had a thought.
I realize that this is not a menthored bug, but i hope its okey to have some questions :)
Turns out that if the position of the button is zero then this if-statement
will not be true, ie metricResult=0 ... Duh!

rv = LookAndFeel::GetInt(LookAndFeel::eIntID_GTKCSDMaximizeButtonPosition,
&metricResult);
if (NS_SUCCEEDED(rv) && metricResult) {
sSystemMetrics->AppendElement(
(nsStaticAtom*)nsGkAtoms::_moz_gtk_csd_maximize_button_position);
}

So i thought id add 1 to the button-value, then subtract it when adding the value to the -moz-box-ordinal-group.
But for some reason it does not look like i can pass a integer to the .css,
seems only boolean values goes through... ie i cant pass a 2 to the .css , only 0 and 1...

I had an idea to do something in the .css like,
@media (-moz-gtk-csd-maximize-button-position: 3) {
.titlebar-close {
-moz-box-ordinal-group: 2;
}
}
and then the same for all 3 buttons, and all 3 positions like :
-moz-gtk-csd-maximize-button-position: 3,
-moz-gtk-csd-maximize-button-position: 2,
-moz-gtk-csd-maximize-button-position: 1
Then set the -moz-box-ordinal-group to the value minus 1. So instead of 1 to 3, it would be 0 to 2

But no that doesnt work either. Im trying to figure out why/where, or even IF, the boolean value comes in...
I have been using the Browser Toolbox to look and change the -moz-box-ordinal-group to move the buttons to the right position.

again, any advice is thankful :)

Attachment #9065211 - Attachment is obsolete: true
Flags: needinfo?(stransky)

Hi Nicklas, don't worry I'm going to look at it. Leaving the needinfo until I have the advice for you.

Hm, I can't get it working either. I'm not sure if we can pass integer value via media or how to transfer the integer value from toolkit to css. Dao, do you have any idea here? Thanks.

Flags: needinfo?(stransky) → needinfo?(dao+bmo)
Comment on attachment 9072271 [details] [diff] [review]
Bug1432090_190614_2300.patch

I suspect `-moz-box-ordinal-group` only accepts integers and you'd need to make it explicitly support your custom keyword. Forwarding to Emilio who knows much more about style system internals.

Also note that we want to move away from `-moz-box` layout in favor of flexbox, so you'd probably want to make this work for `order` as well.
Flags: needinfo?(dao+bmo)
Attachment #9072271 - Flags: feedback?(emilio)
Comment on attachment 9072271 [details] [diff] [review]
Bug1432090_190614_2300.patch

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

Right, so as you've figured out, this cannot work that way. You can special-case these properties and make them parse your custom keyword.

But this sounds like a perfect use-case for CSS environment variables, which we do support. Right now our support for environment variables is a bit rudimentary (all the variables are static atm), but this should be fixable.

Then you don't need to special-case any property parsing and you can do something like:

```
-moz-box-ordinal-group: env(-moz-gtk-csd-minimize-button-position);
```

Or something like that, wdyt? I'm happy to help out introducing something like that. I think it's better than the current approach and would work for every property.

An alternative way to re-use existing setup is via media features and such, but then you need to write media queries for each possible value combination, which is not great at all.

::: browser/themes/linux/browser.css
@@ +25,4 @@
>    --urlbar-separator-color: ThreeDShadow;
>  
>    --chrome-content-separator-color: ThreeDShadow;
> +  --moz-gtk-csd-maximize-button-position: -moz-gtk-csd-maximize-button-position;

This is not used anywhere right? Why is this needed?

@@ +627,4 @@
>    @media (-moz-gtk-csd-minimize-button) {
>      .titlebar-min {
>        -moz-appearance: -moz-window-button-minimize;
> +      -moz-box-ordinal-group: -moz-gtk-csd-minimize-button-position;

How does this possibly work? -moz-ordinal-group doesn't parse any non-integer value.
Attachment #9072271 - Flags: feedback?(emilio) → feedback-

Basically, I think the effort getting the environment variables setup is going to be less, or comparable to, special-casing that property so that it takes an integer.

Thanks Guys for having a look. Ill clean things up, attach a new progress-patch and need info you emilio afterwards.

Im glad that i wasn't way of base, but i've been looking at this a bazillion ways, so the things in the .css was just something i tried...so i knew it was not working and just wanted to get somekindof status

Attached patch Bug1432090_190627_2051.patch (obsolete) — — Splinter Review

Hey Emilio, so i have attached a somewhat cleaned progress-patch. Bare with me :)
Once we have something that works ill use Phabricator to add a patch.

Thought i might be lucky, after looking at https://developer.mozilla.org/en-US/docs/Web/CSS/env
so i just added this to the css for each button, but no luck. :)
-moz-box-ordinal-group: env(-moz-gtk-csd-minimize-button-position);

Guess there is some more parts needed to transfer the integer values to the .css
The initial thought was to use @media glue values...

Attachment #9072271 - Attachment is obsolete: true
Flags: needinfo?(emilio)

Yeah, definitely, this patch needs way more work. I may be able to help and implement this if you want sometime this week. We're going to need this mechanism to get the proper values to here anyhow: https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/servo/components/style/custom_properties.rs#63

Most straight-forward way to implement it would be to hook in here, check for your name before looking at the static ones, and add a function that returns the widget integer you'd like like Gecko_GetLookAndFeelSystemColor does for colors.

But that'd expose the value to content documents, which you don't want to do for fingerprinting. So you'd probably only want to do that if the environment comes from a chrome doc or something like that... So that needs more thought... Maybe storing a bit on CssEnvironment saying whether the document is chrome is enough, but probably just making the CssEnvironment own the map from variable name to variable value is better. That should be able to be done here without too much hassle... But it's probably not a beginners' patch.

Want to give something like that a shot? Otherwise I can take it. We're going to need this mechanism for bug 1503656 anyhow. Just throw me back the ni? so I don't forget ;)

Blocks: 1503656
Flags: needinfo?(emilio) → needinfo?(smurfd)

Thanks, i'll give it a shot.
I am in no rush, i will read through what you wrote above and poke some and see where i will land :)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #23)

Yeah, definitely, this patch needs way more work. I may be able to help and implement this if you want sometime this week. We're going to need this mechanism to get the proper values to here anyhow: https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/servo/components/style/custom_properties.rs#63

Most straight-forward way to implement it would be to hook in here, check for your name before looking at the static ones, and add a function that returns the widget integer you'd like like Gecko_GetLookAndFeelSystemColor does for colors.

If i understand this correctly, i feel as if Some of it is kindof straight forward.
I added my variables in here with a value of 0 : https://searchfox.org/mozilla-central/rev/06bd14ced96f25ff1dbd5352cb985fc0fa12a64e/servo/components/style/custom_properties.rs#74

Also created a similar function like Gecko_GetLookAndFeelSystemColor, which i called Gecko_GetLookAndFeelGTKButtons (maby should call it Gecko_GetLookAndFeelGTKButtonPosition instead, hmm)

What im wondering though is, where we should call my function?
https://searchfox.org/mozilla-central/source/servo/components/style/values/specified/color.rs#248 is where Gecko_GetLookAndFeelSystemColor gets called...

Do we need to create something similar to color.rs and call it from there?

Flags: needinfo?(smurfd) → needinfo?(emilio)

So you probably want to call it on CssEnvironment::get, and it should probably be called something like Gecko_GetLookAndFeelInt or such, nothing gtk specific.

Probably the logic in here should look like:

    if let Some(var) = ENVIRONMENT_VARIABLES.iter().find(|var| var.name == *name) {
         return Some(var);
    }
    if !is_chrome_doc {
        return None;
    }
    if let Some(&(_, int)) = CHROME_ENVIRONMENT_VARIABLES.iter().find(|var| var.0 == *name) {
        let int = bindings::GetLookAndFeelInt(int);
        return Some(make_int_variable_value(int));
    }
    None

There are a few things there that I didn't elaborate on. You need the is_chrome_doc information to end up there, somehow. The most straight forward way to do that is probably making it a member in CssEnvironment.

CHROME_ENVIRONMENT_VARIABLES should probably be a static looking like:

const CHROME_ENVIRONMENT_VARIABLES: [(Atom, i32); 3] = [
    (atom!("-moz-csd-close-button-position"), bindings::eLookAndFeel_YadaYada),
    // ...
];

If we go this route rather than making the CssEnvironment own the variables, then this function needs to return a Option<Cow<'a, VariableValue>> rather than an Option<&VariableValue>, since it can return either a "borrowed" variable from ENVIRONMENT_VARIABLES, or an "owned" one derived from the value of the widget integer.

Flags: needinfo?(emilio)
Attached patch Bug1432090_190725_2151.patch (obsolete) — — Splinter Review

Hey, so i have made the suggested changes. Though, the last part:
"then this function needs to return a Option<Cow<'a, VariableValue>> rather than an Option<&VariableValue>"

That is as close to a foreign language to me as it comes :D

Its probably also why it is not really working as i expected.
Also have not written a line of Rust before, so... ;)

Attachment #9074597 - Attachment is obsolete: true
Flags: needinfo?(emilio)
Comment on attachment 9080726 [details] [diff] [review]
Bug1432090_190725_2151.patch

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

::: layout/style/GeckoBindings.cpp
@@ +701,5 @@
>    return result;
>  }
>  
> +int32_t Gecko_GetLookAndFeelInt(uint32_t aId) {
> +  uint32_t result = 0;

This should be an `int32_t`, probably.

@@ +702,5 @@
>  }
>  
> +int32_t Gecko_GetLookAndFeelInt(uint32_t aId) {
> +  uint32_t result = 0;
> +  LookAndFeel::GetInt(aId, &result);

This is calling into widget which isn't thread-safe, either you make these thread-safe or probably guard with the same thing the other widget things are using, sServoFFILock.

::: layout/style/nsMediaFeatures.cpp
@@ +477,5 @@
> +      (nsStaticAtom*)nsGkAtoms::_moz_gtk_csd_close_button_position);
> +  }
> +
> +  rv = LookAndFeel::GetInt(LookAndFeel::eIntID_GTKCSDMaximizeButtonPosition,
> +                           &metricResult);

Changes to this file can be reverted.

::: servo/components/style/custom_properties.rs
@@ +74,5 @@
>  }
>  
> +lazy_static! {
> +static ref CHROME_ENVIRONMENT_VARIABLES: [(Atom, i32); 3] = [
> +    (atom!("-moz-gtk-csd-close-button-position"), unsafe {bindings::Gecko_GetLookAndFeelInt(bindings::LookAndFeel_eLookAndFeel_GTKCSDCloseButtonPosition)}),

Note that this is going to be called very early and then be immutable, I think it should instead just have the `bindings::LookAndFeel_eLookAndFeel_GTKCSDCloseButtonPosition as i32` number, and call it dynamically when evaluated.

You're also calling the function below, which means that you are effectively doing GetLookAndFeelInt(GetLookAndFeelInt(gtkfoo)), which can only go wrong :)

@@ +84,3 @@
>  impl CssEnvironment {
>      #[inline]
>      fn get(&self, name: &Atom) -> Option<&VariableValue> {

So I assume the main issue is that this is not compiling at all, right? you'd need to change this function to be:

```
fn get<'a>(&'a self, name: &Atom) -> Option<Cow<'a, VariableValue>> {
```

Cow is a "copy on write" object. Basically, it can be a reference, or an owned thing. It'd be equivalent to something like:

```
template<typename T>
class Cow {
  union {
    T mOwned;
    const T* mRef;
  };
  bool mOwns;

  const T& read() const {
    return mOwns ? mOwned : *mRef;
  }

  T& write() {
    if (!mOwns) {
      new (&mOwned) T(*mRef);
      mOwns = true;
    }
    return mOwned;
  }

  ~Cow() {
    if (mOwns) {
      mOwned.~T();
    }
  }
};
```

So when you're grabbing a static VariableValue, you only pass around a reference (Cow::Borrowed, that is, a Cow with mOwns == false).

When you're creating one by calling into widget, you return a Cow::Owned, that is, a Cow with `mOwns == true`.

@@ +84,5 @@
>  impl CssEnvironment {
>      #[inline]
>      fn get(&self, name: &Atom) -> Option<&VariableValue> {
> +        if let Some(var) = ENVIRONMENT_VARIABLES.iter().find(|var| var.name == *name) {
> +            return Some(&var.value);

So this should look like `return Some(Cow::Borrowed(&var.value));`.

@@ +91,5 @@
> +            return None;
> +        }
> +        if let Some(&(_, int)) = CHROME_ENVIRONMENT_VARIABLES.iter().find(|var| var.0 == *name) {
> +            unsafe {
> +            let int = bindings::Gecko_GetLookAndFeelInt(int as u32);

This should probably do something of the sort of:

return Some(Cow::Owned(make_variable!(name.clone(), &format!("{}", int)).value)

Or even better extract the relevant bits of make_variable! to a function or so.

::: servo/components/style/gecko/media_features.rs
@@ +747,4 @@
>          system_metric_feature!(atom!("-moz-gtk-csd-minimize-button")),
>          system_metric_feature!(atom!("-moz-gtk-csd-maximize-button")),
>          system_metric_feature!(atom!("-moz-gtk-csd-close-button")),
> +        system_metric_feature!(atom!("-moz-gtk-csd-minimize-button-position")),

Changes to this file can be reverted.

::: widget/gtk/nsLookAndFeel.h
@@ +24,4 @@
>    virtual void RefreshImpl() override;
>    virtual nsresult NativeGetColor(ColorID aID, nscolor& aResult) override;
>    virtual nsresult GetIntImpl(IntID aID, int32_t& aResult) override;
> +  virtual nsresult GetIntImpl(uint32_t aId, uint32_t& aResult) override;

Thsi is not needed, just cast in the callsite to IntID like this casts to colorID:

https://searchfox.org/mozilla-central/rev/1dfd70469212ef2785d41827c5532c571c784227/layout/style/GeckoBindings.cpp#698

::: widget/nsXPLookAndFeel.cpp
@@ +909,4 @@
>    return NS_ERROR_NOT_AVAILABLE;
>  }
>  
> +nsresult nsXPLookAndFeel::GetIntImpl(uint32_t aId, uint32_t& aResult) {

Same, this function shouldn't be needed.

Hopefully it helps!

Also, docs for Cow are here: https://doc.rust-lang.org/std/borrow/enum.Cow.html

Flags: needinfo?(emilio)

Thanks, it explains alot.

It was (actually) building before though, i had a feeling that i was going in a loop with these rows
(atom!("-moz-gtk-csd-close-button-position"), unsafe {bindings::Gecko_GetLookAndFeelInt(bindings::LookAndFeel_eLookAndFeel_GTKCSDCloseButtonPosition)}),

I have gone through the Rust parts and made the changes, but now it wont build
with this error : expected enum std::borrow::Cow, found reference
on the custom_properties.get() line. and says its because of the env.get() line.
https://searchfox.org/mozilla-central/source/servo/components/style/custom_properties.rs#932-936

Also with this note :
note: expected type std::option::Option<std::borrow::Cow<'_, custom_properties::VariableValue>>
found type std::option::Option<&custom_properties::VariableValue>

So i thought, ill "cast" with a Cow::Borrowed on the custom_properties.get() line... but so far no good.

does this make sense or would you like to see the code?

Flags: needinfo?(emilio)

Yeah, that makes sense.

You need to make both branches return the same thing, and the bottom one gets a reference.

So you probably need to change the bottom branch to be custom_properties.get(&name).map(|v| Cow::Borrowed(&**v)).

Flags: needinfo?(emilio)

Monday update, had written a long post and was about to attach a progress patch.... but during the post i realized a few things, and made a few additions...

I think we are close...

The idea with having these 'nsresult GetIntImpl (uint32_t' instead of 'nsresult GetIntImpl (IntID' with its own Switch statement, was that these values will be between 0 and 2, and those are already occupied by
https://searchfox.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#518-527
(so for now i have those lines commented away, so i can build it :))

So now if i set the value of the button i want in this for loop for each button,
https://searchfox.org/mozilla-central/source/widget/gtk/nsLookAndFeel.cpp#1200-1210

 switch (buttonLayout[i]) {
   case MOZ_GTK_HEADER_BAR_BUTTON_MINIMIZE:
     mCSDMinimizeButton = true;
    mCSDMinimizeButtonPosition = 2;
     break;
   case MOZ_GTK_HEADER_BAR_BUTTON_MAXIMIZE:
     mCSDMaximizeButton = true;
    mCSDMaximizeButtonPosition = 1;
     break;
   case MOZ_GTK_HEADER_BAR_BUTTON_CLOSE:
     mCSDCloseButton = true;
    mCSDCloseButtonPosition = 0;
     break;

it will position them in the order i want them.

i had mCSDMinimizeButtonPosition = i earlier.
That would mean that the order of the buttons in the buttonlayout array is not in the right order, right?
even though i think we debunked that a long time ago....
anyway , some progress in the right direction... i think :)

psssst, it is working now :)
Will clean up, then upload a patch to Phabricator . Then we can discuss :)

This makes it so that Firefox will accomodate the buttonlayout you have set in dconf org.gnome.desktop.wm.preferences.button-layout, in one buttonbox.

Comment on attachment 9080726 [details] [diff] [review]
Bug1432090_190725_2151.patch

this is now obsolete
Attachment #9080726 - Attachment is obsolete: true

hey, realized that the below is all i had to change, compared to what i had in the Phabricator patch.

diff --git a/layout/style/GeckoBindings.cpp b/layout/style/GeckoBindings.cpp
--- a/layout/style/GeckoBindings.cpp
+++ b/layout/style/GeckoBindings.cpp
@@ -703,8 +703,9 @@ nscolor Gecko_GetLookAndFeelSystemColor(

int32_t Gecko_GetLookAndFeelInt(int32_t aId) {
uint32_t result = 0;
++ LookAndFeel::IntID iID = static_cast<LookAndFeel::IntID>(aId);
AutoWriteLock guard(*sServoFFILock);
-- LookAndFeel::GetInt(aId, &result);
++ LookAndFeel::GetInt(iID, &result);
return result;
}

diff --git a/widget/gtk/nsLookAndFeel.cpp b/widget/gtk/nsLookAndFeel.cpp
--- a/widget/gtk/nsLookAndFeel.cpp
+++ b/widget/gtk/nsLookAndFeel.cpp
@@ -704,6 +704,18 @@ nsresult nsLookAndFeel::GetIntImpl(IntID
aResult = mSystemUsesDarkTheme;
break;
}
++ case eLookAndFeel_GTKCSDMaximizeButtonPosition:
++ EnsureInit();
++ aResult = mCSDMaximizeButtonPosition;
++ break;
++ case eLookAndFeel_GTKCSDMinimizeButtonPosition:
++ EnsureInit();
++ aResult = mCSDMinimizeButtonPosition;
++ break;
++ case eLookAndFeel_GTKCSDCloseButtonPosition:
++ EnsureInit();
++ aResult = mCSDCloseButtonPosition;
++ break;
default:

That would then give me this error :

31:17.22 In file included from /home/smurfd/Projects/Source/Mozilla/mozilla-central/obj-x86_64-pc-linux-gnu/widget/gtk/Unified_cpp_widget_gtk2.cpp:20:
31:17.22 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:715:10: error: duplicate case value: 'eIntID_CaretBlinkTime' and 'eLookAndFeel_GTKCSDCloseButtonPosition' both equal '0'
31:17.22 case eLookAndFeel_GTKCSDCloseButtonPosition:
31:17.22 ^
31:17.22 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:518:10: note: previous case defined here
31:17.22 case eIntID_CaretBlinkTime:
31:17.22 ^
31:17.22 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:711:10: error: duplicate case value: 'eIntID_CaretWidth' and 'eLookAndFeel_GTKCSDMinimizeButtonPosition' both equal '1'
31:17.22 case eLookAndFeel_GTKCSDMinimizeButtonPosition:
31:17.22 ^
31:17.22 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:522:10: note: previous case defined here
31:17.22 case eIntID_CaretWidth:
31:17.22 ^
31:17.23 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:707:10: error: duplicate case value: 'eIntID_ShowCaretDuringSelection' and 'eLookAndFeel_GTKCSDMaximizeButtonPosition' both equal '2'
31:17.23 case eLookAndFeel_GTKCSDMaximizeButtonPosition:
31:17.23 ^
31:17.23 /home/smurfd/Projects/Source/Mozilla/mozilla-central/widget/gtk/nsLookAndFeel.cpp:525:10: note: previous case defined here
31:17.23 case eIntID_ShowCaretDuringSelection:
31:17.23 ^
31:17.23 3 errors generated.

My initial thought was ofcourse to try to get my variables to be other values, like 100, 101 and 102, so add 100 first, then subtract 100 from when i would use the ButtonPosition variables. But that felt abit hacky.

So please if you have any ideas :)

Flags: needinfo?(emilio)

You should add the enums to the IntID enum instead, and use LookAndFeel::GetInt(static_cast<IntID>(aID), ...) then remove the new uint32_t API you introduced.

Flags: needinfo?(emilio)

yeah that worked, it was that easy ... <bliiiip> :)

Depends on D73453

Attachment #9085228 - Attachment is obsolete: true
Attachment #9145399 - Attachment is obsolete: true
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b32ad3762d7d
[CSD] Button layout not according to settings - Widget background work r=emilio

Shoot, i just realized i have not Needed info from Dao here in bugzilla, to review https://phabricator.services.mozilla.com/D73454

@Dao if you could please chime in when you have a moment would be awesome. some changes where done after that, but still...
(not sure if its nessesairy to need info both here and ask for review in Phabricator? i see that there is alot in your queue, so no rush)
Basicly it was looking at the CSS things in D73454

Emilio wrote in phabricator :

I'm still a bit torn that we need both the media query and the environment variable, but fine :)

I'd like @dao to confirm whether this approach is fine. Seems we already include titlebar-items.inc.xhtml in two places. I wonder if we can avoid duplicating all the items in another file, maybe by wrapping them in another box?

(sorry for the big bold i had earlier, bugzilla does not like it when you separate lines with --- )

Flags: needinfo?(dao+bmo)

This is based off work by smurfd. But this patch doesn't support buttons
both at the left and right, which simplifies a lot the implementation.

Also, clean-up the existing env variables while at it.

Co-authored-by: Nicklas Boman <smurfd@gmail.com>

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50f07fce131a
Honor GTK button layout. r=stransky,desktop-theme-reviewers,dao
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
status-firefox96: --- → fixed

Probably wrong, this patch should affect only Linux.

Flags: needinfo?(beatrice.acasandrei)
Flags: needinfo?(beatrice.acasandrei)
Keywords: perf-alert

Thanks for letting me know, I've marked the comment as obsolete and removed the perf-alert tag.

Attachment #9145024 - Attachment is obsolete: true
Target Milestone: --- → 96 Branch
No longer blocks: 1503656
Depends on: 1503656
Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: